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

porting @Microsoft SPTAG (native aliases enabled) (continued): ): _mm512_madd_epi16 wrong args, __mmask64 typo, wrong definition, compile warnings #1018

Closed
pabs3 opened this issue May 10, 2023 · 5 comments

Comments

@pabs3
Copy link

pabs3 commented May 10, 2023

@mr-c Thanks for the upload of 0.7.4 to Debian experimental and fixing #961 but unfortunately that wasn't quite enough for me to build SPTAG.

Firstly commit ae545ce broke the definition of _mm512_madd_epi16, it mistakenly added two arguments to the start of the arguments, it should have two arguments not four, like the simde_mm512_madd_epi16 function does.

Secondly there was a typo in commit d850b83, __mask64 should be __mmask64 instead.

Thirdly the typedef uint64_t __mmask64; definition of __mmask64 conflicts with the one in GCC from Debian bookworm, which defines it with typedef unsigned long long __mmask64; in avx512bwintrin.h.

Fourthly the code successfully compiled but with a couple of GCC warnings:

In file included from /usr/include/simde/x86/avx512/mov.h:32,
                 from /usr/include/simde/x86/avx512/dpwssd.h:5,
                 from /usr/include/simde/x86/avx512/4dpwssd.h:5,
                 from /usr/include/simde/x86/avx512.h:33,
                 from /home/pabs/sptag-test-simde/SPTAG.git/AnnService/inc/Core/Common/InstructionUtils.h:16,
                 from /home/pabs/sptag-test-simde/SPTAG.git/AnnService/inc/Core/Common/DistanceUtils.h:11,
                 from /home/pabs/sptag-test-simde/SPTAG.git/AnnService/src/Core/Common/DistanceUtils.cpp:4:
/usr/include/simde/x86/avx512/cast.h: In function 'simde__m512 simde_mm512_castsi512_ps(simde__m512i)':
/usr/include/simde/x86/avx512/cast.h:105:1: note: the ABI for passing parameters with 64-byte alignment has changed in GCC 4.6
  105 | simde_mm512_castsi512_ps (simde__m512i a) {
      | ^~~~~~~~~~~~~~~~~~~~~~~~
/home/pabs/sptag-test-simde/SPTAG.git/AnnService/src/Core/Common/DistanceUtils.cpp: In function 'simde__m512i simde_mm512_add_epi32(simde__m512i, simde__m512i)':
/home/pabs/sptag-test-simde/SPTAG.git/AnnService/src/Core/Common/DistanceUtils.cpp:1051:1: warning: AVX512F vector return without AVX512F enabled changes the ABI [-Wpsabi]
 1051 | }
      | ^
@mr-c
Copy link
Collaborator

mr-c commented May 10, 2023

You are welcome!

Fixes for items 1 and 2 are inbound.

Thirdly the typedef uint64_t _mmask64; definition of _mmask64 conflicts with the one in GCC from Debian bookworm, which defines it with typedef unsigned long long __mmask64; in avx512bwintrin.h.

Looks like C99 & C++11 guarantee that "unsigned long long" is at least 64 bits in any data model, but could be longer. I think defining __mask64 as uint64_t is fine, yes? Is that giving you an error message, or were you (understably) just being extra cautious?

More digging: 64 bits minimum is also in C++03 TR1

https://en.cppreference.com/w/cpp/language/types

Fourthly the code successfully compiled but with a couple of GCC warnings:

/* GCC emits a lot of "notes" about the ABI being different for things
* in newer versions of GCC. We don't really care because all our
* functions are inlined and don't generate ABI. */
#if HEDLEY_GCC_VERSION_CHECK(7,0,0)
#define SIMDE_DIAGNOSTIC_DISABLE_PSABI_ _Pragma("GCC diagnostic ignored \"-Wpsabi\"")
#else
#define SIMDE_DIAGNOSTIC_DISABLE_PSABI_
#endif

Feel free to compile with -Wno-psabi to avoid this, or prefix your usage with SIMDE_DIAGNOSTIC_DISABLE_PSABI_

@pabs3
Copy link
Author

pabs3 commented May 10, 2023 via email

@mr-c
Copy link
Collaborator

mr-c commented May 10, 2023

FYI: In e8390a3 I did add a unsigned long long variant for gcc.

SIMDE_DIAGNOSTIC_DISABLE_PSABI_ is automatically enabled for GCC version 7 and above, since I'm using GCC 12, so it should be enabled already but I am still seeing the two warnings.

Hmm.. I don't know what to say, sorry. Can you try adding SIMDE_DIAGNOSTIC_DISABLE_PSABI_ to the SPTAG source files that generate warnings??

@pabs3
Copy link
Author

pabs3 commented May 11, 2023 via email

@mr-c
Copy link
Collaborator

mr-c commented May 11, 2023

Yeah, I think some of the fixes from SIMDE_DISABLE_UNWANTED_DIAGNOSTICS don't stick for #defined functions.

Thanks again for the feedback!

@mr-c mr-c closed this as completed May 11, 2023
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

No branches or pull requests

2 participants