Repository navigation
Keep simdutf westmere and fallback kernels in x86_64 builds - #266
dylan-conway wants to merge 1 commit into
Conversation
Building with -march=haswell compiles out simdutf's SSE4.2 and scalar kernels, leaving only haswell and icelake. CPUs that execute AVX but do not advertise it via CPUID (e.g. Rosetta 2) then get the "unsupported" implementation, whose methods return constants instead of computing — validate_ascii() returns false for ASCII input and validate_ascii_with_errors() returns (OTHER, 0), which sends callers into infinite loops. Force the lower-tier kernels back in for x86_64 so runtime dispatch always has a working floor. CPUs with AVX2/AVX-512 still select the haswell/icelake kernels, so this has no effect on machines that advertise those features.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds preprocessor directives in SIMDUTF.cpp that force inclusion of the Westmere and fallback SIMDUTF implementation kernels on x86_64 builds, ensuring runtime dispatch always has a valid implementation available instead of potentially selecting an unsupported, constant-returning implementation. ChangesSIMDUTF Kernel Configuration
Sequence Diagram(s)Not applicable; this change is a compile-time configuration adjustment with no observable runtime call flow to diagram. Estimated code review effort: Low Suggested labels: WTF, build Suggested reviewers: none identified from the provided context Poem 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward preprocessor fix to keep simdutf's westmere/fallback kernels compiled in on x86_64.
Extended reasoning...
Overview
This PR adds a small preprocessor block to Source/WTF/wtf/SIMDUTF.cpp that defines SIMDUTF_IMPLEMENTATION_WESTMERE=1 and SIMDUTF_IMPLEMENTATION_FALLBACK=1 on x86_64 before including the simdutf amalgamation. I verified against simdutf_impl.cpp.h that these macros are the correct override points: the amalgamation's own #ifndef blocks default westmere/fallback to 0 when SIMDUTF_CAN_ALWAYS_RUN_HASWELL is set (i.e., under -march=haswell), and pre-defining them to 1 re-enables those kernels without touching dispatch priority.
Security risks
None. This is a compile-time configuration change that enables additional (already-present) SIMD kernel implementations in the simdutf amalgamation. No new code paths are introduced beyond what simdutf already ships; runtime dispatch order is unchanged.
Level of scrutiny
Low. The change is 17 lines (mostly explanatory comment), gated behind CPU(X86_64), and uses #ifndef guards so it composes with any external overrides. It is purely additive at build time — CPUs advertising AVX2/AVX-512 continue to select haswell/icelake, while environments like Rosetta 2 that don't advertise AVX now get a working westmere/fallback floor instead of the unsupported singleton.
Other factors
The PR description includes manual verification under Rosetta 2 with and without the defines. The bug-hunting system found no issues. The comment in the code accurately captures the caveat that these kernels are still compiled at the TU's -march, so this targets translators rather than genuinely pre-AVX hardware. No outstanding reviewer comments on the timeline.
Preview Builds
|
1 participant
The x64 artifacts are built with
-march=haswell, which makes simdutf's amalgamation compile out its SSE4.2 (westmere) and scalar (fallback) kernels. On CPUs that execute AVX but don't advertise it via CPUID — Rosetta 2 — runtime dispatch then finds no usable implementation and silently installs theunsupportedsingleton, whose methods return constants instead of computing:validate_ascii()returns false for ASCII input andvalidate_ascii_with_errors()returns(OTHER, 0). In Bun this sends the latin1→utf8 conversion into an infinite loop during startup, so the x64 build hangs under Rosetta 2 on any JS execution.This forces the SSE4.2 and scalar kernels back in on x86_64. Dispatch priority is unchanged (icelake → haswell → westmere → fallback), so CPUs that advertise AVX2/AVX-512 select the same kernels as before. The re-enabled kernels are still compiled at the TU's
-march, so this helps translators that execute AVX without advertising it, not CPUs that genuinely cannot execute AVX.Verified by compiling the amalgamation with
-march=haswellwith and without these defines and running under Rosetta 2: without them the active implementation isunsupportedandvalidate_asciireturns false on pure-ASCII input; with them it iswestmereand all results are correct. arm64 builds are unaffected (the block is gated onCPU(X86_64)).