Skip to content

Fix riscv SIMD target detection ## Summary - recognize __riscv before x86 / ARM SIMD probing in cglm's public headers; - keep riscv64 on the existing generic non-SIMD path instead of inheriting host x86 SIMD state.#494

Open
carlosqwqqwq wants to merge 1 commit into
recp:masterfrom
carlosqwqqwq:riscv-cglm

Conversation

@carlosqwqqwq

Copy link
Copy Markdown

Why

cglm already has a conservative generic path that can serve as the baseline for riscv64, but its current preprocessing can inherit host x86 SIMD builtins during simulated or cross-target validation. That leakage affects both backend selection and matrix alignment decisions, which makes a forced riscv64 build select x86-specific code paths even though no RISC-V SIMD backend exists yet.

What changed

  • Add explicit target-architecture detection in include/cglm/simd/intrin.h so __riscv is recognized before x86 or ARM probing.
  • Restrict x86 intrinsics setup in intrin.h to x86 targets only, instead of relying on raw host __SSE*__ and __AVX__ macros alone.
  • In include/cglm/common.h, clear host x86 SIMD builtins when the target is forced to __riscv so later headers and alignment logic do not inherit the wrong architecture state.
  • Update README.md to document that riscv64 currently uses cglm's generic non-SIMD path unless a dedicated RISC-V backend is added.

Verification

  • Ran the native CMake configuration successfully:
    • cmake -S . -B build-native-min -G Ninja -DCGLM_SHARED=OFF -DCGLM_STATIC=ON -DCGLM_USE_TEST=OFF -DCMAKE_BUILD_TYPE=Release
  • Ran the native build successfully:
    • cmake --build build-native-min --parallel 4
  • Compiled a forced-riscv64 smoke translation unit that includes cglm/cglm.h and exercises glm_mat4_mulv, with explicit assertions that CGLM_SIMD_x86, CGLM_SSE_FP, CGLM_SSE2_FP, CGLM_AVX_FP, __SSE__, and __AVX__ must not be present.
  • Preprocessed cglm/cglm.h with -D__riscv=1 -D__riscv_xlen=64 and confirmed the result only reports CGLM_ARCH_RISCV, with no x86 SIMD feature macros.
  • Preprocessed cglm/cglm.h on the native x86_64 host and confirmed the existing x86 SIMD path still reports CGLM_ARCH_X86, CGLM_SIMD_x86, CGLM_SSE_FP, and CGLM_SSE2_FP.
  • Ran the simulated riscv64 CMake build successfully:
    • cmake --build build-riscv-config --parallel 4
  • Checked build-riscv-config/build.ninja and confirmed the generated compile flags do not include -msse* or -mavx*.
  • Verified real riscv64 cross-compilation and execution in Docker (ubuntu:24.04):
    • built a minimal consumer including cglm/cglm.h and exercising glm_vec3_add and glm_mat4_mulv with host gcc;
    • rebuilt the same consumer with riscv64-linux-gnu-gcc;
    • confirmed the resulting binary is ELF64 / Machine: RISC-V via file and readelf -h;
    • ran the riscv64 binary successfully with qemu-riscv64-static -L /usr/riscv64-linux-gnu.

Notes

This is a conservative portability patch. It does not add RVV or any other RISC-V SIMD implementation. The goal is to make riscv64 select cglm's existing generic path correctly and to stop host x86 SIMD state from leaking into simulated or cross-target validation.

@recp

recp commented Jul 4, 2026

Copy link
Copy Markdown
Owner

I think we must not undef compiler's macros. It can affect user code included after cgl.

Is this because these are used in headers like vec4.h? Some paths use defined(CGLM_SIMD) to use glmm api, while others check __SSE__, __SSE2__, AVX, wasm, or wasm_simd128` directly:

CGLM_INLINE
void
glm_vec4_add(vec4 a, vec4 b, vec4 dest) {
#if defined(__wasm__) && defined(__wasm_simd128__)
  glmm_store(dest, wasm_f32x4_add(glmm_load(a), glmm_load(b)));
#elif defined( __SSE__ ) || defined( __SSE2__ )
  glmm_store(dest, _mm_add_ps(glmm_load(a), glmm_load(b)));
#elif defined(CGLM_NEON_FP)
  vst1q_f32(dest, vaddq_f32(vld1q_f32(a), vld1q_f32(b)));
#else
  dest[0] = a[0] + b[0];
  dest[1] = a[1] + b[1];
  dest[2] = a[2] + b[2];
  dest[3] = a[3] + b[3];
#endif

Maybe converting these to use CGLM defined SIMD macros:

CGLM_SSE_FP
CGLM_SSE2_FP
CGLM_AVX_FP
CGLM_NEON_FP
CGLM_SIMD_WASM

could help better?

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.

2 participants