Repository navigation
simd: support aggregate signatures in AVX2 multiversioning - #2799
Merged
xushiwei merged 3 commits intoOct 14, 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: aggregate signatures in AVX2 multiversioning
Solid, carefully-reasoned change. Four review passes (code quality, performance, security, documentation accuracy) found no correctness, memory-safety, or ABI defects, and no stale documentation.
What I verified holds up:
- Pipeline reorder (
internal/build/build.gocompilePackageModule): movingllvmfmv.Runto afterlowerLargeAggregates+TransformModulecorrectly makesmusttaildispatchers forward the caller's already-loweredbyval/sretstorage instead of materializing an aggregate in a forwarding frame the tail jump releases. No post-FMV amd64 pass re-materializes aggregates (lowerWasmAggregateCopiesis a no-op off wasm).assertAggregateTailForwardingis a strong regression guard (rejectsalloca, non-musttail, lostbyval/sret). - Recursive type walks in
cl/simd_fmv.go(check) andinternal/llvmfmv/fmv.go(supportedType) are correctly bounded: theseenmap +hasVectorshort-circuit + by-value-only descent prevent infinite recursion, and opaque structs / wider-than-128-bit vector leaves are rejected. isQueryWin64i32-receiver acceptance stays gated behind the mandatoryllgo.cpu.query="x86.avx2"attribute, so a non-query function cannot be misclassified.- Docs/comments (
fmv.goRuncomment,windows_vector.go,test/simd/README.md) all match the new ordering and capability; no leftover "aggregates not yet specialized" wording.
The inline notes below are optional low-severity nits only — nothing blocking.
Additional non-blocking observation (pre-existing code, not changed here): in internal/llvmfmv/fmv.go specialize, query folding only collects CallInst queries, so a query reached via InvokeInst would be left unfolded. The AVX2 query is a pure non-unwinding read never emitted as an invoke today, so this is purely theoretical — mentioning only in case invoke-form queries ever become possible.
| hasVector := false | ||
| sig := source.Signature | ||
| check := func(t types.Type) { | ||
| seen := make(map[types.Type]bool) |
Contributor
There was a problem hiding this comment.
Nit (compile-time, low): markSIMDFMV runs once per amd64 function (every function in every package). On this path it unconditionally does lookupSIMD(source, "amd64") (line 35) and allocates make(map[types.Type]bool) (line 39), even though the vast majority of signatures contain no struct/array types and the walk visits few or zero types. Consider a cheap pre-filter (skip when the signature has no aggregate/named types) or allocating seen lazily on the first aggregate. Pure overhead on the common path; not blocking.
| for _, name := range []string{"main.guarded", "main.array", "main.pair", "main.one", "main.large"} { | ||
| fast := mod.NamedFunction(name + ".__llgo_fmv_avx2") | ||
| if fast.IsNil() { | ||
| t.Fatalf("aggregate function %s has no AVX2 implementation", name) |
Contributor
There was a problem hiding this comment.
Nit: this message (and the loop at ~line 75) calls every entry an "aggregate function", but main.pair is register-only and main.large is a non-SIMD aggregate. They acquire an AVX2 variant because they call the trusted AVX2() query and become FMV roots, not because they carry a vector in their signature. A neutral phrase (e.g. "function %s has no AVX2 implementation") plus a one-line comment distinguishing "root because it calls the query" from "has a vector in its signature" would keep a regression report accurate.
|
|
||
| func assertAggregateTailForwarding(t *testing.T, fn llvm.Value) { | ||
| t.Helper() | ||
| if strings.Contains(fn.String(), "alloca") { |
Contributor
There was a problem hiding this comment.
Nit: strings.Contains(fn.String(), "alloca") is a string-level heuristic over the IR dump — a future unrelated alloca or a value/metadata name containing that substring would produce a confusing failure. The structural checks below (musttail, matching function type, forwarded params, preserved sret/byval) are the strong assertions. Consider scanning instructions with IsAAllocaInst() to match their rigor. Low priority.
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/simd-fmv-aggregates-20261012
branch
from
October 12, 2026 15:50
7d4479c to
0a7d9ae
Compare
zhouguangyuan0718
force-pushed
the
codex/simd-fmv-aggregates-20261012
branch
from
October 13, 2026 14:09
ecfa13f to
8fbd3c2
Compare
cpunion
approved these changes
Oct 14, 2026
This was referenced Oct 14, 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.
CPU-guarded SIMD functions with slices, strings, structs, arrays or multiple results were excluded from AVX2 function multiversioning. Enable these signatures, including nested SIMD128 values and cross-package direct calls, while retaining the existing cached dispatch and effective CPU-feature policy.
Finish large-aggregate and target ABI lowering before FMV creates dispatchers. This lets
musttailcalls forward the caller's argument/result storage with the same physical signature andbyval/sretattributes. Recognize the ABI-lowered Win64 CPU-query receiver, and identify SIMD128 values nested by value in structs and arrays when promising cross-package variants.Validation:
cpu.avx2=off,cpu.avx=off,cpu.fma=offandcpu.all=offfor every binary. Runtime coverage includes initialization, value-copy isolation, overlapping input/result assignments, cross-package calls, indirect entries, closures and stack traces.The initial Go CI failures came from
lib/cimporting its 128-bit integer implementation throughc/types, which pulledmath/big/fmtand LLGo runtime dependencies into the runtime-free fixtures. Rebased onto main02f07cee7withgoplus/lib v0.6.3, which removes that dependency from the base C bindings. Both previously failing tests (TestPCLNExternalPureCLibraryIdentityRetentionIntegrationandTestRuntimeFreeNativeMetadata, off/full) now pass, together with all SIMD LLVM/assembly cases. The SIMD patch is unchanged by the rebase; no assertions or tests were disabled.The full short compiler/build suites and complete macOS arm64, Linux amd64 and Windows/Wine SIMD matrices above pass on
0a7d9ae96. Commit7472773d4adds boundary tests for malformed CPU-query signatures and aggregates whose layout is not yet defined; the FMV unit suite passes, and compiler implementation files are unchanged. Codecov reports 100% patch coverage against a 95.46% target.Hosted validation completed for the preceding head
7472773d411fd7f7d0417312a5f3e0c3e59117d6: all 12 workflows succeeded, with 102 successful checks and the release step skipped as expected for a pull request. This includes the complete Linux, macOS, Windows MSVC and Windows MinGW Go CI suites, Windows embedded GDB, native Windows amd64/arm64/386 installation tests, and release artifact validation.Review refinements in
ecfa13fd1defer visited-type map creation until an aggregate is traversed, restrict intrinsic lookup to bodyless functions with vector signatures, clarify the query-root test diagnostic, and identify dispatcher allocations by LLVM instruction kind. CPU-query marking remains independent of vector signatures. The targeted compiler SIMD, build SIMD/FMV, and query/aggregate boundary tests all pass; that head subsequently had five Go workflow failures from the stale native-debug SSA test APIs in the PR merge baseline.This continues #2748 and remains limited to amd64 AVX2 specialization of SIMD128 code; it does not add SIMD256/512 or arbitrary feature combinations. Windows native execution remains covered by hosted CI rather than the local Wine matrix.
Rebased onto current main
1bbfc1f75db405854cfc3c49ed8104c0741c2231(successful main Go workflow) at head8fbd3c2e1fa6fe621cf0f610c9337fbe1e2bb853. All three commits are patch-equivalent according togit range-diff. Fullinternal/llvmfmv,internal/cabi, andssatests passed after rebase, together with compiler/build SIMD tests, the three-target aggregate LLVM/assembly matrix, and runtime-elision regressions. Hosted CI completed successfully for this exact head: 102 successful checks and one release step skipped as expected for a pull request.This is an earlier roadmap step, but #2802's portable bridge primitives are an independent change based directly on main; #2802 does not require this PR to compile or pass its own tests.