-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add AVX2 support #23035
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
Add AVX2 support #23035
Conversation
site/source/docs/porting/simd.rst
Outdated
| * - _mm_i64gather_epi64 | ||
| - ❌ scalarized | ||
|
|
||
| All the 128-bit wide instructions from AVX2 instruction set are listed. Only a small part of the 256-bit AVX2 instruction set are listed, most of the 256-bit wide AVX2 instructions are emulated by two 128-bit wide instructions. |
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.
Can you wrap text at 80 here
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.
Done
| * found in the LICENSE file. | ||
| */ | ||
|
|
||
| #ifndef __emscripten_immintrin_h__ |
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.
Let's use a different macro that gets undefined after all the relevant includes in immintrin.h. Otherwise, we won't emit this error if the user does something like this:
#include <immintrin.h>
#include <avxintrin.h>
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.
Yes, in this case, it won't emit error message, but I think it's suitable.
Actually, include avxintrin.h directly will miss prerequisites and cause compile error. Include immintrin.h can avoid this error.
immintrin.h includes the necessary prerequisites and choose the *intrin.h corresponding to options(-mavx, -mavx2...).
so in the following code, if -mavx option is specified, #include <avxintrin.h> does nothing due to header guards, it's valid code, the behavior is consistent with LLVM.
#include <immintrin.h>
#include <avxintrin.h>
system/include/compat/avx2intrin.h
Outdated
| // This may cause an out-of-bounds memory load since we first load and | ||
| // then mask, but since there are no segmentation faults in Wasm memory | ||
| // accesses, that is ok (as long as we are within the heap bounds - | ||
| // a negligible limitation in practice) | ||
| // TODO, loadu or load, 128-bit align? |
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 this cause ASan failures?
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.
The snippet is ported from old code in avxintrin.h, I am not sure, I will check that.
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.
The implementation is changed, scalarized version is used now, it's more compliant with specifications.
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.
I'm fine with this landing once @sbc100's documentation comments are addressed and the tests are passing, etc.
e838094 to
1551fa2
Compare
dbf0628 to
4f6db96
Compare
da4ba4f to
50f8885
Compare
|
Looks like this was lgtm'd pending tests passing, and they just passed, landing. |
It's successor of PR #22430, one 256-bit AVX2 intrinsic can be also emulated by two 128-bit intrinsics.