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

MSVC support #596

Merged
merged 18 commits into from May 23, 2023
Merged

MSVC support #596

merged 18 commits into from May 23, 2023

Conversation

anthony-linaro
Copy link
Contributor

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.

@jserv jserv requested review from AymenQ and howjmay and removed request for marktwtn May 17, 2023 13:35
@anthony-linaro anthony-linaro mentioned this pull request May 17, 2023
tests/impl.cpp Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
tests/impl.cpp Outdated Show resolved Hide resolved
tests/impl.cpp Outdated Show resolved Hide resolved
tests/impl.cpp Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
@@ -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__)
Copy link
Collaborator

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?

Copy link
Contributor Author

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

sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
@invertego
Copy link
Collaborator

It's not a huge deal, it'd be nice if this built cleanly at /W4. Here's what's remains:

sse2neon.h(6468,9): warning C4310: cast truncates constant value
sse2neon.h(8258,39): warning C4100: 'a': unreferenced formal parameter
sse2neon.h(8260,39): warning C4100: 'b': unreferenced formal parameter
sse2neon.h(8261,35): warning C4100: 'lb': unreferenced formal parameter
sse2neon.h(8271,39): warning C4100: 'a': unreferenced formal parameter
sse2neon.h(8272,35): warning C4100: 'la': unreferenced formal parameter
sse2neon.h(8273,39): warning C4100: 'b': unreferenced formal parameter
sse2neon.h(8356,50): warning C4100: 'b': unreferenced formal parameter
sse2neon.h(8367,39): warning C4100: 'a': unreferenced formal parameter

@jserv jserv requested a review from invertego May 22, 2023 08:43
@jserv
Copy link
Member

jserv commented May 22, 2023

@anthony-linaro, Please resolve conflicts.

@jserv
Copy link
Member

jserv commented May 23, 2023

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?

@invertego
Copy link
Collaborator

invertego commented May 23, 2023

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 __clang__ in addition to _M_ARM64 to prevent this assumption from applying to clang.

@anthony-linaro
Copy link
Contributor Author

anthony-linaro commented May 23, 2023 via email

@anthony-linaro
Copy link
Contributor Author

@jserv This should now (hopefully) be resolved

@jserv
Copy link
Member

jserv commented May 23, 2023

@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))),
                   ^

@anthony-linaro
Copy link
Contributor Author

@jserv Let's try that again

@DLTcollab DLTcollab deleted a comment from anthony-linaro May 23, 2023
@anthony-linaro
Copy link
Contributor Author

@jserv Formatting issues should now be solved also

@jserv jserv merged commit b390e8a into DLTcollab:master May 23, 2023
13 checks passed
@jserv
Copy link
Member

jserv commented May 23, 2023

Thank @anthony-linaro and @invertego for this great effort!

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

Successfully merging this pull request may close these issues.

None yet

3 participants