-
Notifications
You must be signed in to change notification settings - Fork 212
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
MSVC support #596
MSVC support #596
Conversation
This resolves issue DLTcollab#384
@@ -6018,6 +6090,7 @@ FORCE_INLINE __m64 _mm_abs_pi8(__m64 a) | |||
// Concatenate 16-byte blocks in a and b into a 32-byte temporary result, shift | |||
// the result right by imm8 bytes, and store the low 16 bytes in dst. | |||
// https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm_alignr_epi8 | |||
#if defined(__GNUC__) && !defined(__clang__) |
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.
Why does GCC need special treatment 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.
IIRC (I didn't actually write this particular bit, someone else did), the updated version didn't work in GCC
It's not a huge deal, it'd be nice if this built cleanly at /W4. Here's what's remains:
|
@anthony-linaro, Please resolve conflicts. |
The proposed change caused MinGW breakage: $ aarch64-w64-mingw32-clang++ -o tests/common.o -Wall -Wcast-qual -I. -march=armv8-a+fp+simd -std=gnu++14 -c -MMD -MF tests/common.o.d tests/common.cpp
In file included from tests/common.cpp:1:
In file included from tests/common.h:5:
./sse2neon.h:838[9](https://github.com/DLTcollab/sse2neon/actions/runs/5046362677/jobs/9063430352?pr=596#step:4:10):[11](https://github.com/DLTcollab/sse2neon/actions/runs/5046362677/jobs/9063430352?pr=596#step:4:12): error: use of undeclared identifier '__crc32ch'
crc = __crc32ch(crc, v); @invertego and @anthony-linaro, can you check? |
This change assumes the availability of CRC and AES intrinsics on aarch64, but clang's arm_neon.h only declares them if the requisite architectural features are enabled. Adding "+crc+aes" to ARCH_CFLAGS in the Makefile will make these errors go away. I suppose that's sufficient so long as we don't actually care about building sse2neon (specifically for Windows) with these features disabled. Otherwise, it will be necessary to check |
As far as I can tell (I did add it in a comment) there does not exist a
supported WoA platform without crypto extensions (people have it running on
some unsupported ones like old Windows Phones such as the Lumia 950, but
those are rare/unsupported, and we can discount them).
I will modify the makefile for Windows as suggested.
|
@jserv This should now (hopefully) be resolved |
It seems that crypto extension was not properly handled. $ aarch64-w64-mingw32-clang++ -o tests/common.o -Wall -Wcast-qual -I. -march=armv8-a+fp+simd -std=gnu++14 -c -MMD -MF tests/common.o.d tests/common.cpp
In file included from tests/common.cpp:1:
In file included from tests/common.h:5:
./sse2neon.h:8[9](https://github.com/DLTcollab/sse2neon/actions/runs/5055092682/jobs/9070866019?pr=596#step:4:10)16:20: error: use of undeclared identifier 'vaeseq_u8'
vaesmcq_u8(vaeseq_u8(vreinterpretq_u8_m[12](https://github.com/DLTcollab/sse2neon/actions/runs/5055092682/jobs/9070866019?pr=596#step:4:13)8i(a), vdupq_n_u8(0))),
^ |
@jserv Let's try that again |
@jserv Formatting issues should now be solved also |
Thank @anthony-linaro and @invertego for this great effort! |
This resolves issue #384.
Much of this is based on the initial effort done by @invertego in said issue, however it adds some extras, and ensures that all the tests pass correctly.
This was tested on my Thinkpad X13s running Windows 11 22H2 (22621.1702), and using SDK 10.0.22621.