-
Notifications
You must be signed in to change notification settings - Fork 972
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
Simd #133
Simd #133
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Rebased to resolve conflict in snappy-internal.h |
@atdt , would you pls help to review? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JunHe77 thank you very much for this contribution!
Would you like to address the feedback below, or would you prefer that I take care of it while going through our merge process?
@@ -36,13 +36,44 @@ | |||
namespace snappy { | |||
namespace internal { | |||
|
|||
#if !defined(SNAPPY_HAVE_BMI2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for realizing we need these #defines and #includes in the header, for the V128
type alias.
Can you please move this block outside namespaces?
I think that #including C system headers in a C++ namespace is undefined behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out, will update.
snappy-internal.h
Outdated
using V128 = uint8x16_t; | ||
#endif | ||
|
||
#if SNAPPY_HAVE_BMI2 | ||
inline uint32_t zero_highbits_u32(uint32_t v, int n) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain the benefit of wrapping _bzhi_u32
with this function?
Until I learn more, I'm inclined to suggest that we remove the abstraction. It's currently used in exactly one place, and it's only defined when SNAPPY_HAVE_BMI2
. So, it's effectively a less-known alias for the _bzhi_u32
intrinsic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well received, will update.
After SHUFFLE code blocks are refactored, "tmmintrin.h" is missed, and bmi2 code part will have build failure as type conflicts. Signed-off-by: Jun He <jun.he@arm.com> Change-Id: I7800cd7e050f4d349e5a227206b14b9c566e547f
Signed-off-by: Jun He <jun.he@arm.com> Change-Id: I3fade568ff92b4303387705f843d0051d5e88349
@JunHe77 Thank you very much for the revisions! I'll get this PR through the merge process in our internal repository. When the process completes, the PR will be merged. |
No description provided.