[libc-commits] [PATCH] D137868: [libc] Fix builtin definition for memory functions

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/string/memory_utils/op_x86.h:29
+// functions in case the builtin is not present.
+#ifndef _mm512_cmpneq_epi8_mask
 #define _mm512_cmpneq_epi8_mask(A, B) 0
----------------
gchatelet wrote:
> michaelrj wrote:
> > gchatelet wrote:
> > > lntue wrote:
> > > > How will this work if `_mm512_cmpneq_epi8_mask` is defined as a function or compiler builtin instead of macro?  Will `defined(_mm512_cmpneq_epi8_mask)` also return false, and the macro will overwrite the real function calls?
> > > So technically the right way to do it is to rely on the `__has_builtin` intrinsic.
> > > It is available on clang forever but is quite recent on gcc, it requires gcc 10 (another bummer to have to support both gcc and clang)
> > > https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/_005f_005fhas_005fbuiltin.html
> > `__has_builtin` doesn't work because this isn't the name of the builtin. From `avx512bwintrin.h`:
> > 
> > ```
> > #define _mm512_cmp_epi8_mask(a, b, p) \
> >   ((__mmask64)__builtin_ia32_cmpb512_mask((__v64qi)(__m512i)(a), \
> >                                           (__v64qi)(__m512i)(b), (int)(p), \
> >                                           (__mmask64)-1))
> > ```
> > 
> > And `#ifndef` doesn't work either because sometimes these are defined as functions, from `avx2intrin.h` (on the same system):
> > 
> > ```
> > static __inline__ int __DEFAULT_FN_ATTRS256
> > _mm256_movemask_epi8(__m256i __a)
> > {
> >   return __builtin_ia32_pmovmskb256((__v32qi)__a);
> > }
> > ```
> > 
> > But it appears that the problems I was having trying to build on windows previously can be fixed by using the proper extension name (AVX512BW vs AVX512F), this builds clean on both and (afaict) uses the correct builtins.
> Ha yes indeed, they are sometimes macros sometimes builtins...
> 
> Anyways it's great to know that using the proper preprocessor definitions fixes it!
> 
> BTW do we have windows CI machines? I don't think our bazel config currently handles windows build but I can have a look if needed.
We don't currently have Windows CI working, and the Windows builds I've tried have all been cmake/ninja.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137868/new/

https://reviews.llvm.org/D137868