Replace __cpuid_check for AVX512 with compiler builtin cpu features#630
Replace __cpuid_check for AVX512 with compiler builtin cpu features#630r-devulap wants to merge 2 commits intodatastax:mainfrom
Conversation
Replaces manual CPUID bit parsing in jvector_simd_check.c with __builtin_cpu_supports and adds __builtin_cpu_init(). This corrects missing XSAVE runtime checks, preventing #UD exceptions on systems where 512‑bit register state saves are disabled.
|
Do we support building with MSVC? In that case, neither |
|
Looks like only GCC is supported at the moment. Refer jextract_vector_simd.h. |
We seem to be testing on windows CI where the native AVX512 module is not built. Do we want it to work on Windows or is it not important enough to worry about. |
|
I don't think we should worry about it too much at the moment. If we want to support the native SIMD module on Windows there are more extensive changes needed anyway. If it seems important enough, maybe just add a comment over the sections that you feel might complicate future attempts at porting to Windows? |
yeah, that's fine by me. Was just curious about MSVC support. The current way of using __cpuid_check also doesn't work on MSVC, so not much will change with this patch. |
jshook
left a comment
There was a problem hiding this comment.
I would like to see some evidence of testing here, even though this should just work. If it doesn't, then the risk goes up dramatically for downstream surprises.
|
How is the determination to load the native module by jvector made today? Is it based on the caller of the library (i.e. Astra or Opensearch), or the library determines it itself based on machine environment? If it is determined by the library, it would be good to include the cpuId detection for ARM, as many of us use mac's for development and we can take advantage of advanced vector instructions. The latter may be out of scope for this PR. |
The vector code written using Panama API should already generate the appropriate vector instructions on an ARM CPU. The JVM handles both the runtime checks and JIT generation of NEON vector code. AFAIK, JVector does not have any native ARM code and hence doesn't require any CPUID detection for ARM. |
Then what is the purpose of the JVector-native module, if Panama is already handling ARM and x86 instructions generation?
If the decision to load Panama vs JVector native module is based on Jvector library logic, then should we consider including ARM instructions in native module as well? |
AFAIK, the routines in the native AVX‑512 module can’t be written using the Panama Vector API (I still need to determine the exact limitations—work in progress). If that turns out to be true, then yes, we’ll need to implement the ARM equivalents |
Fixes #629.
This pull request simplifies AVX-512 feature detection logic in the
jvector_simd_check.cfile by replacing manual CPUID bit checks with built-in compiler functions (available only on gcc and llvm compilers). The earlier code was missing XSAVE support at runtime. On VMs where 512‑bit register state saving is disabled, this can result in #UD faults when executing AVX‑512 instructions.CPU feature detection improvements:
__builtin_cpu_supportsfor each required feature, improving code readability and portability.__builtin_cpu_init()to ensure proper initialization before feature checks.check_compatibilitytocheck_avx512_compatibility.