Repository navigation
compiler: preserve AVX2 specialization across Midway calls - #2835
Merged
cpunion merged 4 commits intoOct 19, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: preserve AVX2 specialization across Midway calls
The production change is small, correct, and well-covered. simdFMVVector cleanly separates the two recognition paths — the official archsimd shape via SIMDVectorShape, and the simd/internal/bridge peer type validated by package path, a vector-shaped namesake in simd/archsimd, and types.Identical on the underlyings. That structural identity check is the right mechanism (a bridge type can't pass SIMDVectorShape directly because its v128 tag lives in archsimd, not its own package), and it only recognizes types for FMV marking without touching lowering or ABI. The types.Unalias handling, the negative cases (wrong package, pointer wrapper, wider type, mismatched underlying), and the cross-package/aggregate/array/closure/method/interface/generic/initialization fixtures are thorough. No security concerns, and the README prose accurately matches the implemented behavior.
A few minor, non-blocking suggestions are left inline. None block merge.
Other notes (no inline location)
- Test readability (low priority): The embedded Go source in
internal/build/simd_midway_fmv_test.go(lines ~24–51) and the negative-case table incl/simd_bridge_test.go:52-61pack several operations per line. Adding per-case labels to the negative table and de-densifying the embedded source would make future failure localization easier. The embedded source files are also written by ranging amap[string]string, so the write order is nondeterministic — harmless today, but a[]{name, source}slice would be deterministic at no cost. - Pre-existing, out of this PR's scope:
markSIMDFMVis invoked fromcl/instr.go:739on every function reference (outside theaFn == nilcache-miss guard), not once per function. This PR's bridge path adds a small gated import scan + scope lookup that is therefore repeated per reference. It's tightly gated behind the twosimd/internal/bridgepath checks, so there's no asymptotic blow-up, but if that per-reference call is ever tightened to the cache-miss path it would remove the redundancy for free.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
zhouguangyuan0718
force-pushed
the
codex/midway-fmv-followup-20261017
branch
from
October 18, 2026 03:51
96958ae to
0e0e784
Compare
(cherry picked from commit 2cc3c5b)
Collaborator
Author
|
@fennoai /review Please re-review the latest head The bridge peer-package lookup now has linear control flow, and the earlier missing-peer/import coverage tests remain in place. FMV bridge tests and Linux/Darwin/Windows O0/O2 Midway IR tests pass locally. The previous inline findings have individual responses and dispositions. All three SIMD PRs also include the isolated native-debug harness fix from commit 2cc3c5b in #2849: disable external symbol lookup and stream LLDB progress. Their preceding Ubuntu jobs exhausted the job limit in native source-debug acceptance after SIMD tests passed; the fixed harness passes locally. The preceding coverage checks passed (this is not a claim about the fresh CI run). Fresh CI has been triggered for this head and is still pending. Please report any remaining correctness, regression, or performance issues against this exact revision. Review only; no code changes are requested. |
Contributor
Re-review: PR #2835 — preserve AVX2 specialization across Midway callsReviewed the exact revision Verdict: No correctness, regression, or performance issues against this revision. The follow-up cleanly resolves the prior inline findings. Approve-equivalent; only one optional wording nit remains. Follow-up changes verified
Soundness of the core changeThe bridge-type eligibility is conjunctive and tight: PerformanceNo concern. For all non-SIMD code the function returns after an O(1) type assertion plus one or two package-path string compares; the import scan + lookup + Minor (optional, non-blocking)
CI for this head is still pending per the PR note; this review reflects static analysis of the exact revision, not the fresh CI run. No security concerns, and all prior inline findings are addressed or appropriately dispositioned. |
cpunion
approved these changes
Oct 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Midway's 128-bit portable representations are defined
simd/internal/bridgetypes. FMV currently recognizes onlyarchsimdvectors, so an AVX2-specialized portable function calling a vector-containing aggregate or array helper in another package keeps calling the baseline entry. This recognizes the official bridge representation and preserves AVX2 specialization across those calls, without changing its storage or calling convention.Based on main
c41bc55d9, following #2819. Bridge eligibility requires a matching official archsimd type and identical underlying storage; emulated representations, wider vectors, unrelated packages, and incompatible storage remain ineligible. CPU queries remain independent of Midway representation selection: emulation does not imply AVX2 is off, and SIMD128 does not imply AVX2 is on. Propagating additional Midway entry guarantees and pruning feature combinations remain separate follow-up work.Validation:
cl,ssa, andinternal/buildtests; fullinternal/llvmfmv,internal/midway, andinternal/packagestests.simd=0/128with AVX2, AVX, FMA, and all optional CPU features disabled.go vet ./...reports diagnostics in unchanged test fixtures (context, assignment order, unsafe pointer, JSON, and syscall lock-copy cases).Docker/Wine results are emulated execution, not native hardware qualification. Hosted CI will run on this PR.