Activate SSE2 test on windows #563

serge-sans-paille · 2021-09-14T05:23:17Z

No description provided.

cyb70289 · 2021-09-18T11:30:42Z

Turns out AVX2 is always enabled in the source code
https://github.com/xtensor-stack/xsimd/blob/master/test/CMakeLists.txt#L92
@serge-sans-paille

serge-sans-paille · 2021-09-18T17:33:16Z

thanks @cyb70289 . I now have a clean reproducer for your bug, is that okay if I cherry-pick your fix in this branch and adjust it?

cyb70289 · 2021-09-19T01:37:49Z

Sure, no problem

serge-sans-paille · 2021-09-19T11:58:49Z

All green. Thanks @cyb70289 for the hints! and cc @JohanMabille for the merge.

cyb70289 · 2021-09-19T13:46:20Z

include/xsimd/config/xsimd_config.hpp

+#if defined(_M_X64) || (defined(_M_IX86_FP) && _M_IX86_FP >= 2)
+#define XSIMD_WITH_SSE2 1
+#endif


If we only enable SSE2 here, then SSE3,SSE4 will not be supported by xsimd if built with msvc.
I think this is a problem and will still break many code.
MSVC X64 must support SSE4_2 (actually /arch:SSE2 is an invalid option for msvc x64).
What about define XSIMD_WITH_SSE4_2 if _M_X64?

According to the doc there's indeed no explicit flag for SSE3 and later (based on https://docs.microsoft.com/fr-fr/cpp/build/reference/arch-x86?view=msvc-160) but I wonder if it's actually supported if no arch flag is specified ?

X64 msvc only supports /arch:AVX?, https://docs.microsoft.com/en-us/cpp/build/reference/arch-x64?view=msvc-160

My understanding is /arch:XXX anables msvc to auto vectorize c++ code using at most the XXX instructions.
But it doesn't prevent us from using intrinsics directly. Compiler should supports all level simd instructions.

MSVC does not define __SSEn__ macros.

cyb70289

Thanks @serge-sans-paille !

serge-sans-paille · 2021-09-23T18:31:52Z

@cyb70289 unfortunately, I'm not able to reproduce the windows error locally (on xsimd::extract_pair) could you have a look?

cyb70289 · 2021-09-24T01:24:37Z

include/xsimd/arch/xsimd_ssse3.hpp

      batch<T, A> extract_pair(batch<T, A> const& self, batch<T, A> const& other, std::size_t i, ::xsimd::detail::index_sequence<I, Is...>) {
        if(i == I) {
-          return _mm_alignr_epi8(self, other, sizeof(T) * I);
+          return _mm_alignr_epi8(other, self, sizeof(T) * (I));


other and self swapped?

serge-sans-paille · 2021-09-24T05:44:03Z

yeah, note that this makes the validation pass :-) So unless the validation itself is not correctly written...

serge-sans-paille · 2021-09-24T07:08:10Z

@cyb70289 can you confirm e.g. on your code base that current implementation is correct, i.e. that the tests are well-written?

cyb70289 · 2021-09-24T07:14:45Z

@cyb70289 can you confirm e.g. on your code base that current implementation is correct, i.e. that the tests are well-written?

I use extract_pair in my simd code. It works correctly on both x86 and arm.
So I guess maybe xsimd test is not correct. Will have a look at my side.

cyb70289 · 2021-09-24T07:32:17Z

Looks to me this test code to generate expected result is wrong:
https://github.com/xtensor-stack/xsimd/blob/master/test/test_extract_pair.cpp#L35-L42

_mm_alignr_epi8(lhs, rhs, 1) should return (from lsb to msb) rhs[1] rhs[2] ... rhs[15] lhs[0],
but the test code expects lhs[1] lhs[2] ... lhs[15] rhs[0].
https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_mm_alignr_epi8&expand=302

cyb70289 · 2021-09-25T04:51:09Z

include/xsimd/arch/xsimd_neon.hpp

                if (n == I)
                {
-                    return vextq_u8(lhs, rhs, I);
+                    return vextq_u8(rhs, lhs, I);


Ah, vextq_xx is indeed different from _mm_alignr_xx 👍

serge-sans-paille · 2021-09-25T05:49:27Z

All green! cc @JohanMabille this one costed me quite a lot of effort :-)

JohanMabille · 2021-09-27T09:10:42Z

Congratulation on this one!

serge-sans-paille mentioned this pull request Sep 14, 2021

Fix SSE build failure with MSVC #562

Closed

serge-sans-paille force-pushed the fix/test-sse2-windows branch from 7b9c5dc to bb66d89 Compare September 17, 2021 21:23

serge-sans-paille force-pushed the fix/test-sse2-windows branch from bb66d89 to 12066d5 Compare September 18, 2021 12:39

cyb70289 reviewed Sep 19, 2021

View reviewed changes

serge-sans-paille force-pushed the fix/test-sse2-windows branch from b4e097e to 3118d0a Compare September 23, 2021 07:07

serge-sans-paille and others added 2 commits September 23, 2021 09:54

Activate SSE2 test on windows

12747a3

Fix SSE build failure with MSVC

2f02ee2

MSVC does not define __SSEn__ macros.

serge-sans-paille force-pushed the fix/test-sse2-windows branch from 3118d0a to 2f02ee2 Compare September 23, 2021 07:56

cyb70289 approved these changes Sep 23, 2021

View reviewed changes

cyb70289 reviewed Sep 24, 2021

View reviewed changes

serge-sans-paille force-pushed the fix/test-sse2-windows branch from bde7a37 to 343f881 Compare September 24, 2021 11:18

Fix extract_pair test and generic implementation

a9f3ccb

serge-sans-paille force-pushed the fix/test-sse2-windows branch from 343f881 to a9f3ccb Compare September 24, 2021 15:35

Fix extract_pair implementation on arm

f4f2945

cyb70289 reviewed Sep 25, 2021

View reviewed changes

JohanMabille merged commit aa54369 into master Sep 27, 2021

JohanMabille deleted the fix/test-sse2-windows branch September 27, 2021 09:10

cyb70289 mentioned this pull request Sep 28, 2021

[SSE] xsimd sse code failed to build with msvc #561

Closed

Activate SSE2 test on windows #563

Activate SSE2 test on windows #563

Uh oh!

Conversation

serge-sans-paille commented Sep 14, 2021

Uh oh!

cyb70289 commented Sep 18, 2021

Uh oh!

serge-sans-paille commented Sep 18, 2021

Uh oh!

cyb70289 commented Sep 19, 2021

Uh oh!

serge-sans-paille commented Sep 19, 2021

Uh oh!

cyb70289 Sep 19, 2021

Choose a reason for hiding this comment

Uh oh!

serge-sans-paille Sep 19, 2021

Choose a reason for hiding this comment

Uh oh!

cyb70289 Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

cyb70289 left a comment

Choose a reason for hiding this comment

Uh oh!

serge-sans-paille commented Sep 23, 2021

Uh oh!

cyb70289 Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

serge-sans-paille commented Sep 24, 2021 via email

Uh oh!

serge-sans-paille commented Sep 24, 2021

Uh oh!

cyb70289 commented Sep 24, 2021

Uh oh!

cyb70289 commented Sep 24, 2021

Uh oh!

cyb70289 Sep 25, 2021

Choose a reason for hiding this comment

Uh oh!

serge-sans-paille commented Sep 25, 2021

Uh oh!

JohanMabille commented Sep 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants