Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernels: add 8i_x2_saturated_sum_8i #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evilynux
Copy link
Contributor

Includes implementations for AVX2, SSE2 and NEON along the generic kernel.
Beware, the NEON kernel is untested (don't have the HW handy).

Copy link
Member

@marcusmueller marcusmueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix the include guard; in that process, I'd recommend updating the year.

@@ -0,0 +1,277 @@
/* -*- c++ -*- */
/*
* Copyright 2015 Free Software Foundation, Inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong year?


#endif /* LV_HAVE_AVX2 for unaligned */

#endif /* INCLUDED_VOLK_8s_SATURATED_SUM_8s_UNALIGNED8_H */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds wrong!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This #endif needs to be removed.



#ifndef INCLUDED_volk_8i_x2_saturated_sum_8i_a_H
#define INCLUDED_volk_8i_x2_saturated_sum_8i_a_H
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh-oh- multiple include guard defines! That means that from line 171-end, things should never be compiled, since line 72 defines that macro!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 lines as Marcus notes need to be removed.

@marcusmueller
Copy link
Member

pinging @evilynux here: You still up for fixing this code in a spare minute?

jessica-iwamoto added a commit to jessica-iwamoto/volk that referenced this pull request Jul 17, 2018
…1-add-avx2-to-fm_detect-conv_k7_r2 to next

* commit '028652df04a978772b9aa61ac2f5b2ed351dc98c':
  added avx2 to volk 8u x4 conv k7 r2 kernel
  Added AVX2 to fm_detect, still working on adding to conv_k7_r2
@noc0lour
Copy link
Member

noc0lour commented Mar 7, 2019

@evilynux are you still interested in getting the changes merged?

@evilynux
Copy link
Contributor Author

@noc0lour My apologies, I'm completely overwhelmed and have been unable to find the required time to fix this. I don't see this changing any time soon... :-/

@noc0lour
Copy link
Member

No problem, let's see if I can look at this anytime soon. Thanks for your work until now!

@noc0lour noc0lour self-assigned this Apr 24, 2019
@noc0lour
Copy link
Member

@evilynux otherwise can you hit [email protected] an email to do a quick CLA so I can pick your work up in a spare minute?

@noc0lour noc0lour added the CLA label Jul 20, 2019
Copy link
Contributor

@michaelld michaelld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this PR's addition, but the macro protection needs to be cleaned up! And, this PR needs to be rebased to the current GIT master. And I believe we need a CLA (license assignment) in place for large of an addition of code, per a prior comment about contacting Ben Hilburn who handles CLAs.


#endif /* LV_HAVE_AVX2 for unaligned */

#endif /* INCLUDED_VOLK_8s_SATURATED_SUM_8s_UNALIGNED8_H */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This #endif needs to be removed.



#ifndef INCLUDED_volk_8i_x2_saturated_sum_8i_a_H
#define INCLUDED_volk_8i_x2_saturated_sum_8i_a_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 lines as Marcus notes need to be removed.


#endif /* LV_HAVE_NEON for aligned */

#endif /* INCLUDED_VOLK_8s_SATURATED_SUM_8s_ALIGNED8_H */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this #endif should be INCLUDED_volk_8i_x2_saturated_sum_8i_u_H ... same as that starting the header. case matters!

@jdemel
Copy link
Contributor

jdemel commented Nov 28, 2020

@evilynux Is there any update on the CLA?

@jdemel
Copy link
Contributor

jdemel commented Feb 21, 2021

@evilynux I assume there's still no update on your CLA?

Anyways, we adopted a new system: DCO. Thus, you only need to sign off your commits. git commit -s for new commits. The details are explained in DCO.txt. With git commit -s you add a Signed off by ... line to the end of your commit message. You can add

I got this approach to sign previous commits from stackoverflow

git rebase --exec 'git commit --amend --no-edit -n -s' -i HEAD~1

I updated the option -s to lower case. Also, HEAD~1 should refer to your one commit. It would be great if you could rebase your PR onto current master. If you could fix the review comments, then we could merge this PR. I'd be happy to finally be able to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants