-
Notifications
You must be signed in to change notification settings - Fork 187
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
Enable vectorized implementations on x86 #115
base: main
Are you sure you want to change the base?
Conversation
highwayhash/vector256.h
Outdated
@@ -287,7 +287,12 @@ class V256<uint64_t> { | |||
|
|||
// Broadcasts i to all lanes. | |||
HH_INLINE explicit V256(T i) | |||
#if HH_ARCH_X86 | |||
// _mm_cvtsi64_si128 is not available on x86 | |||
: V256(i,i,i,i) {} |
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.
This likely won't work on MSVC. How about we use _mm256_set_epi32 instead?
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.
This works on MSVC toolkit 170 but I've changed it to use _mm_insert_epi32, which should generate better code I think.
Nice work :) We don't officially support 32-bit but these seem like reasonable changes that we can keep in. FYI we haven't yet set up the infra for syncing github with our internal code (the previously used approach has been deprecated). I will be out until Jan31 and hope we can get to this in mid-Feb. |
Nice, thanks for making the change :) |
I hope I'm not bothering you by reminding you about this now. |
Sorry we still haven't merged this. In the meantime there is another implementation by John Platts which uses Highway and thus has much better support for various targets including x86. Would you like to switch to that one? |
Enable SSE4.1 and AVX2 implementations to be built and used on x86, and avoid unnecessary uses of uint64_t for small values