Repository navigation
Conversation
Highway enables its AVX10_2 target when the compiler is clang 23 or newer (hwy/detect_targets.h, HWY_BROKEN_AVX10_2). The LLVM 23 upgrade therefore added an eighth copy of every dispatched kernel on x64: HWY_TARGETS went from 7120 to 7128. Each of the 60 AVX10_2 kernels compiles to the same instructions as its AVX3_SPR copy (57) or its AVX3_DL copy (3). Highway's detection sets AVX3_SPR for every CPU it sets AVX10_2 for, so such a CPU dispatches to AVX3_SPR when AVX10_2 is not compiled. Add HWY_AVX10_2 to HWY_DISABLED_TARGETS, next to the SVE targets that are off since clang 22, and remove the AVX10_2 entries from the x64 baseline allowlists. linux-x64 release, stripped: 80,995,912 -> 80,934,472 bytes.
|
Status: ready for review. How this was checked, on linux-x64 at f063852:
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe Highway build configuration disables the AVX10_2 target. The x64 and Windows x64 static-baseline allowlists remove AVX10_2 symbols and update the corresponding section counts. ChangesAVX10_2 target and baseline symbols
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Disabling AVX10_2 preserves dispatch coverage through AVX3_SPR without an identified current-head regression. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a one-token build-flag change plus the matching allowlist pruning.
What was reviewed:
disabledTargetsin scripts/build/deps/highway.ts flows unquoted into-D...for both Highway's own sources and bun's compile (scripts/build/bun.ts:250), so+/-in the value need no shell escaping on any platform.- Both x64 allowlists: every
N_AVX10_2entry is gone, no other entries changed, and the updated section counts (234/10/5/45) match the actual entry counts; aarch64 allowlist untouched, as expected for an x86-only bit. - Could not fetch the pinned Highway commit here to read
HWY_AVX10_2directly; the pre-existingN_AVX10_2allowlist entries and the 7128-to-7120 (=1<<3) delta both point to the macro existing, and a wrong name would fail every CI build at the first Highway include rather than silently.
Extended reasoning...
The change touches only the Highway dependency build script (one string constant and a comment) and two x64 baseline-ISA allowlists, removing 56 AVX10_2 kernel entries; no runtime source, tests, or security-sensitive surface is involved. The define is consumed verbatim as a -D flag by both the Highway direct build and bun's compile, so the added +HWY_AVX10_2 reaches every Highway-including TU. Allowlist edits are purely subtractive and internally consistent with their section counts, and the static scan would flag any AVX10_2 code that reappeared. The runtime effect is confined to CPUs with AVX10.2, which Highway also flags as AVX3_SPR-capable, so dispatch falls to an existing kernel copy; the changed files have no CODEOWNER and there are no outstanding reviewer objections.
|
@robobun wake up!! |
|
@robobun wake up!! |
|
I am here. This is the state of the PR at a5790f8. Nothing is pending from my side. CI
Size, from the
The baseline of that step is main at c86bd18, which is 3 commits after the merge base of this branch. The linux-x64 number agrees with my local measurement (-61,440 bytes). I added the table to the Notes of the description. Against main today (a4f1429)
What it needs
|
This branch has not been deployed
3 participants
Problem
AVX10_2. Highway enables it from clang 23 (hwy/detect_targets.h:198).HWY_TARGETSwent from 7120 to 7128.AVX3_SPRcopy, 3 to theAVX3_DLcopy. The linker cannot fold them, because the dispatch tables hold their addresses.Fix
HWY_AVX10_2toHWY_DISABLED_TARGETSinscripts/build/deps/highway.ts, beside the SVE targets from build: pin Highway's aarch64 target set (no scalable SVE/SVE2 with clang ≥ 22) #40921.HWY_TARGETSis 7120 again.N_AVX10_2entries from each x64 baseline allowlist. The static scan fails if that code comes back.AVX3_SPRfor every CPU it setsAVX10_2for (hwy/targets.cc:336). Such a CPU dispatches toAVX3_SPR.test/js/bun/util/highway-strings.test.tson debug and release builds. AlsoescapeHTML.test.jsandstringWidth.test.ts.Background
HWY_DISABLED_TARGETSremoves targets at compile time.AVX3_SPR(89% identical to other copies). Left out: it changes what runs on Sapphire Rapids today.Downsides
JsonIndexImpl,XmlIndexImplandXmlIndex16Implrun a different copy than before. Not measured: no such hardware was available.Notes
Section deltas of the size line above:
.text-46,080,.rodata-16,384.Stripped size per target, CI release builds of this branch against main at c86bd18 (3 commits after the merge base of this branch):
How the copies were compared:
llvm-objdump -dof everyN_<target>kernel in the CI canary build of f063852, with branch targets made function-relative, callee names stripped of their target namespace, and RIP-relative and jump-table addresses masked. After that, all 50,417 bytes ofAVX10_2kernels equal another target's copy. No instruction mnemonic appears in anN_AVX10_2function that does not also appear in anN_AVX3*function.Per-target kernel bytes (symbol sizes, 60 kernels each):
HWY_TARGETSfrom the preprocessor, clang 23.1.2, vendored Highway:--target=x86_64-linux-gnu -march=nehalem--target=x86_64-pc-windows-msvc -march=nehalem--target=aarch64-linux-gnu -march=armv8-a+crc--target=arm64-apple-macos13 -mcpu=apple-m1x86 target bits are 0 to 14 and Arm target bits are 15 to 29, so the x86 bit has no effect on Arm. The define uses
+and not|, because the bits are disjoint and|needs quoting under cmd.exe.History: the allowlists already had
N_AVX10_2entries before #42851, and #42851 added more. The 1.4.2 release binary (clang 21) has noN_AVX10_2symbol, so those older entries matched nothing at that time.Debug build:
indexOfAny / lastIndexOfAny: 2, 3 and 16-byte setsinhighway-strings.test.tstakes 9 to 13 s under debug with ASAN on the build host and passes the 5 s default timeout neither with nor without this change. It passes with--timeout 60000(11 pass, 56,393 expect() calls). The debug binary has 774N_AVX10_2symbols before and 0 after.[policy-decision:dep] gate passed · iteration 0 · 3 files touched
passes on PR (with fix)
Dependency/toolchain change; no test proof to run.diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file