Fix ICX segfault caused by unconditional ARM64 SIMD wrapper types - #621
Fix ICX segfault caused by unconditional ARM64 SIMD wrapper types#621stefanatwork wants to merge 3 commits into
Conversation
The __m128_wrapper/__m128i_wrapper structs in common/simd/arm/simd_wrapper_types.h were introduced to work around an MSVC/ARM64-specific overload ambiguity (__m128 and __m128i both alias __n128 on that platform). However, the wrapper was applied unconditionally in vboolf4_sse2.h, vfloat4_sse2.h, vint4_sse2.h, and vuint4_sse2.h, replacing the plain __m128/__m128i union members (and their implicit conversions) with a wrapper struct on ALL platforms, including x86. This introduced non-trivial-constructor types placed inside unions alongside raw int/float arrays (type punning that is technically UB) plus a class exposing both a value-returning const conversion operator and a non-const reference conversion operator simultaneously - a known overload-resolution footgun. This is suspected to have caused miscompilation under ICX's aggressive optimizer, leading to SegFaults in embree_verify during the nightly-linux-DEBUG-ICX-AVX/AVX2/AVX512 CI jobs shortly after the new ARM64 support merged (all three ISA widths failed identically, pointing to a compiler/ABI issue rather than an ISA-specific bug). Fix: scope the wrapper-based union member and conversion operators to MSVC+ARM64 only, and restore the original plain __m128/__m128i union member with implicit conversions for all other builds (x86 and non-MSVC ARM64, e.g. Linux/macOS aarch64 via clang/gcc, which already worked without the wrapper before this change). The explicit m128i()/m128d() accessor methods are kept available on both paths so call sites elsewhere in these files that already use them continue to compile unchanged. Validated: compiles cleanly and all embree_verify tests pass with Intel ICX/ICPX 2026.0.0 (RelWithDebInfo, AVX, TBB tasking, testing intensity 4 - matching CI settings as closely as possible locally: 6346 passed / 0 failed), Clang 21.1.8 (Debug, AVX2: 8476 passed / 0 failed), and GCC (header compile smoke test). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addresses issue introduced in b282335 |
There was a problem hiding this comment.
Pull request overview
This PR addresses ICX (Intel oneAPI) runtime segfaults introduced after adding Windows ARM64 support by scoping ARM64/MSVC-specific SIMD wrapper usage to only the platform that needs it, restoring the original plain __m128 / __m128i union members on other toolchains/architectures to avoid UB-prone union storage of non-trivial wrapper types.
Changes:
- Restricts
__m128_wrapper/__m128i_wrapperunion members to#if defined(_MSC_VER) && defined(_M_ARM64)in the affected SSE2 vector headers. - Restores plain
__m128/__m128iunion members and their reference conversion operators on non-MSVC/ARM64 builds. - Clarifies rationale via comments (and keeps
.m128i()/.m128d()accessors available for call-site compatibility).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| common/simd/vboolf4_sse2.h | Restores plain __m128 union member and non-wrapper conversions on non-MSVC/ARM64; keeps wrapper path only for MSVC ARM64. |
| common/simd/vfloat4_sse2.h | Makes the __m128_wrapper union member and v.data conversions MSVC/ARM64-only; otherwise uses plain __m128. |
| common/simd/vint4_sse2.h | Makes the __m128i_wrapper union member and v.data conversions MSVC/ARM64-only; otherwise uses plain __m128i. |
| common/simd/vuint4_sse2.h | Makes the __m128i_wrapper union member and v.data conversions MSVC/ARM64-only; otherwise uses plain __m128i. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
svenwoop
left a comment
svenwoop
left a comment
There was a problem hiding this comment.
In general I do not understand why you introduced these wrapper types in the first place. This looks like solving the problem in two different ways. We added the explicit conversions which should fix all issues don't they?
Also the wrapper is implemented twice actually, and both implementations are identical.
| #else | ||
| __forceinline operator const __m128&() const { return v; } | ||
| #if !defined(__EMSCRIPTEN__) | ||
| __forceinline operator const __m128i() const { return _mm_castps_si128(v); } |
There was a problem hiding this comment.
Why do we add this operator only in that case? We should now only go through new functions.
| #if defined(_MSC_VER) && defined(_M_ARM64) | ||
| __forceinline operator const __m128&() const { return v.data; } | ||
| #if !defined(__EMSCRIPTEN__) | ||
| __forceinline const __m128i m128i() const { return _mm_castps_si128(v.data); } |
There was a problem hiding this comment.
One could just remove the .data here and the functions should also work without the wrapper, the wrapper has conversion operators to the base type
Problem
Since the merge of the new ARM64 support (
b28233586"Add Windows ARM64 MSVC support"), the Linux ICX nightly CI jobs (nightly-linux-DEBUG-ICX-AVX,-ICX-AVX2,-ICX-AVX512) started SegFaulting inembree_verify/embree_verify_i2, crashing partway throughregression_static_memory_monitorright afterinstance_arraysreported[FAILED]. All three ISA widths failed identically, which points to a compiler/codegen issue rather than an ISA-specific bug.Root cause
common/simd/arm/simd_wrapper_types.hdefines__m128_wrapper/__m128i_wrapper/__m128d_wrapperstructs to work around a genuine MSVC/ARM64-specific overload ambiguity (__m128and__m128iboth alias__n128there). However, the wrapper was applied unconditionally invboolf4_sse2.h,vfloat4_sse2.h,vint4_sse2.h, andvuint4_sse2.h, replacing the plain__m128/__m128iunion members (and their implicit conversions) with the wrapper struct on all platforms, including x86.This introduced:
int/floatarrays — technically UB type punning.constconversion operator and a non-constreference conversion operator — a known overload-resolution footgun.This combination is suspected to have caused miscompilation under ICX's aggressive optimizer.
Fix
Scope the wrapper-based union member and conversion operators to
#if defined(_MSC_VER) && defined(_M_ARM64)only. Restore the original plain__m128/__m128iunion member with implicit conversions for all other builds (x86, and non-MSVC ARM64 such as Linux/macOS aarch64 via clang/gcc, which already worked fine without the wrapper prior to this change). The explicit.m128i()/.m128d()accessor methods remain available on both paths so existing call sites elsewhere in these files continue to compile unchanged — keeping the diff minimal and surgical.Validation
EMBREE_MAX_ISA=AVX, TBB tasking, testing intensity 4 (matching CI settings as closely as possible locally): compiles cleanly,embree_verify: 6346 passed / 0 failed.embree_verify: 8476 passed / 0 failed.Caveat: I was unable to reproduce the exact segfault locally (different CPU/exact ICX version than the CI Docker image), so this fix is based on strong code-review evidence (the UB pattern, and the fact the crash is ISA-independent) plus confirmation that no test regressions were introduced, rather than a direct before/after repro of the crash itself.