Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: extend aggregate-copy lowering to all architectures
This PR makes LowerWasmAggregateCopies run on every target (not just wasm) and adds a copyMultiElementArrays path that lowers any array with more than one element, regardless of size. The core motivation (LLVM scalarizes first-class loads of multi-element arrays like NTT polynomials) is sound and the memmove/memcpy selection is correct.
One correctness concern stands out and is worth confirming before merge: the frontend GC-safepoint predicate in cl/gcroot.go and the ABI lowering predicate in internal/abi/large.go now disagree about which loads become heap-allocating snapshots. See the inline comments.
Summary of findings
- P1 (correctness):
isGCSafepointstill uses a size-only threshold (≥4 KiB) whileisLargeCopynow snapshots multi-element arrays of any size — the snapshot path heap-allocates viaruntime.AllocU(a new safepoint) that the root plan may not account for. - P2 (performance): small fixed-size arrays (e.g.
[2]int,[4]byte) that previously stayed in registers on native targets are now forced through memmove or, in the multi-store case, a heap allocation. - P3 (clarity):
Wasm-prefixed names/filename now describe an architecture-independent pass; deadgoarchparameter inwasm_copies.go. - Tests: the new multi-element-array cases only exercise the single-adjacent-store memmove fast path (no
GCRoots, noAllocU); the small-array + safepoint interaction is untested.
| size := p.prog.SizeOf(p.type_(load.Type(), llssa.InGo)) | ||
| if size > llabi.MaxImplicitStackVarSize || | ||
| (p.prog.Target().GOARCH == "wasm" && size >= llabi.MinWasmAggregateCopySize) { | ||
| if size > llabi.MaxImplicitStackVarSize || size >= llabi.MinWasmAggregateCopySize { |
Contributor
There was a problem hiding this comment.
P1 — safepoint predicate no longer mirrors the lowering predicate.
This condition treats an aggregate load as a safepoint only when size > MaxImplicitStackVarSize (64 KiB) or size >= MinWasmAggregateCopySize (4 KiB) — i.e. effectively size >= 4 KiB.
But isLargeCopy in internal/abi/large.go now returns true for any multi-element array via the new copyMultiElementArrays path, with no size floor. When such a small array (e.g. [2]*T = 16 bytes) has multiple stores/extracts, transformStoredLoad takes the snapshot path and calls allocResult → runtime.AllocU, a real heap allocation and therefore a new GC safepoint that was absent from the frontend root plan. The pass roots its own snapshot and the copy source, but unrelated Go pointers live only across that newly-inserted alloc get no root slot here, since functionHasGCSafepoint/isGCSafepoint won't flag a sub-4 KiB array. A collection during AllocU could then reclaim a still-referenced object (use-after-free; collector is non-moving so it's premature-free rather than relocation).
The comment just above states the intent is to "account for that added allocation now," so this branch should mirror isMultiElementArray: also return true for *types.Array with length > 1 regardless of size. Consider factoring a single shared predicate so the two passes cannot drift again.
| } | ||
|
|
||
| func (l largeAggregateLowerer) isMultiElementArray(typ llvm.Type) bool { | ||
| return typ.TypeKind() == llvm.ArrayTypeKind && typ.ArrayLength() > 1 |
Contributor
There was a problem hiding this comment.
This predicate has no size floor, so [2 x i64] (16 bytes) and similar tiny arrays now qualify as "large copies" on every architecture. Two consequences:
- Correctness: in the multi-store/extract case these go through the allocating snapshot path (
AllocU), which the size-only safepoint check incl/gcroot.godoes not recognize — see the inline comment there. - Performance: small fixed-size arrays that previously stayed as register-resident first-class SSA values on native targets are now forced through
memmove(single-store) or a heap allocation (multi-store), defeating SROA/value-forwarding with no scalarization benefit for arrays that fit in a few registers.
Consider gating copyMultiElementArrays on a minimum size (or a small register-width threshold) rather than ArrayLength() > 1, which preserves the fix for large NTT-style arrays while leaving small arrays in registers.
| if goarch != "wasm" { | ||
| return 0 | ||
| } | ||
| func lowerWasmAggregateCopies(_ string, td llvm.TargetData, mod llvm.Module, config abi.AggregateLoweringConfig) int { |
Contributor
There was a problem hiding this comment.
P3 — dead parameter. With the goarch != "wasm" early return removed, the first parameter is now unused (_ string) yet both call sites still pass ctx.buildConf.Goarch. It misleadingly suggests the pass still branches on architecture. Consider dropping the parameter and updating call sites.
Relatedly, the Wasm-prefixed names (lowerWasmAggregateCopies, LowerWasmAggregateCopies, MinWasmAggregateCopySize, and the wasm_copies.go filename) now describe an architecture-independent pass. Renaming to arch-neutral names — or at least a doc comment noting the pass now runs on all targets and "Wasm" is retained for historical reasons — would prevent a future reader from reintroducing a wasm guard. (The Wasm bool config field is still legitimately wasm-specific.)
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 |
visualfc
force-pushed
the
fix/can-ssa-array-copies
branch
from
September 29, 2026 00:11
f1bffcb to
7c34fc2
Compare
Collaborator
Author
|
Addressed the P1/P2 notes: multi-element array copies are now gated on cmd/compile's CanSSA size limit (4 pointer words, 32 bytes on 64-bit). |
2 tasks done
visualfc
force-pushed
the
fix/can-ssa-array-copies
branch
from
September 29, 2026 01:36
021533a to
44fee7c
Compare
Collaborator
Author
|
Rebased onto #2690 so |
LLVM default<Os> scalarizes first-class loads of mid-size array and struct values. Packages such as go/ast/edge and crypto/internal/fips140/mlkem spend seconds in SLP/ISel on copies a few KiB above the old host threshold. Reuse the existing Wasm 4KiB copy pass on every target: load/store of aggregates >= 4KiB become memmove. Return sret and the native C ABI stay at the 64KiB MaxImplicitStackVarSize limit.
Drop the unused goarch argument now that the copy pass runs on every target, and rename the pass to LowerAggregateCopies so the old Wasm prefix does not imply it is wasm-only. config.Wasm still selects the GC-root frame layout. Simplify isGCSafepoint to size >= 4KiB. Run the same post-C-ABI copy pass on native C export wrappers, which previously skipped it.
LowerAggregateCopies inserts runtime.AllocU for multi-use aggregate loads. In functions with debug info, LLVM requires inlinable calls to have a !dbg location. Copy the load/call's debug loc onto AllocU, matching the existing memcpy/memmove handling.
cmd/compile never represents arrays with more than one element as SSA values; copies stay in memory as OpMove. LLGo emitted first-class load/store of types such as [256 x i32], and LLVM default<Os> then spent seconds in SLP and AArch64 ISel on ML-DSA NTT. After C ABI lowering, rewrite those array copies to memmove. Loads used as call arguments are left unchanged so register passing of small arrays stays C-compatible. Return sret and MaxImplicitStackVarSize are unchanged.
cmd/compile's CanSSA limit is 4 pointer words. Arrays larger than that are copied in memory; smaller multi-element arrays stay first-class so they can remain in registers and do not allocate snapshots. Share ShouldSnapshotAggregateLoad with the frontend safepoint predicate so a heap snapshot cannot appear without a matching GC root plan.
Multi-use copies of arrays larger than 4 pointer words were lowered with AllocU on every target. On Wasm that inserted extra heap safepoints into the precise collector, which stalled runtime.GC and JS value finalizers. Keep lowering those arrays on Wasm. Snapshots smaller than 4KiB use an entry alloca; AllocU and GC roots remain only for copies at the existing 4KiB threshold, matching the host/Wasm split that already passed CI.
visualfc
force-pushed
the
fix/can-ssa-array-copies
branch
from
September 30, 2026 02:22
c4dd95e to
d028c5a
Compare
cpunion
reviewed
Oct 3, 2026
| } else { | ||
| b.SetInsertPointBefore(first) | ||
| } | ||
| return b.CreateAlloca(typ, "") |
Collaborator
There was a problem hiding this comment.
[P2] Allow disjoint stack snapshots to reuse storage
Each sub-4 KiB snapshot receives a separate entry-block alloca without lifetime markers, so even mutually exclusive branches retain the sum of their snapshot slots.
Reproducer: eight switch cases, each doing v := Source; Mutate(&Source, caseID); Dest = v, where Source/Dest are [256]uint32 and the non-inlined mutator modifies the array. Ordinary Go source reproduces native snapshot stack reservation growing from 1,024 to 8,192 bytes. In the corresponding LLVM fixture at -Os (LLVM 22), the ARM64 frame grows from 1,056 to 8,256 bytes, and the Wasm32 backend reserves 8,192 linear-stack bytes instead of zero on the base.
Copies remain correct, but stack usage scales with the total number of snapshots rather than their peak simultaneous lifetime, increasing fixed-stack exhaustion risk. Please retain loop-safe entry allocation while providing lifetimes/slot reuse or a frame-budget fallback, and add a mutually exclusive-branch stack-usage regression test.
Collaborator
Author
There was a problem hiding this comment.
Addressed: sub-4 KiB snapshots still use a loop-safe entry alloca. Disjoint live ranges of the same type now share one slot and get llvm.lifetime.start / llvm.lifetime.end (LLVM 22 pointer-only form).
An 8-way switch of [256]uint32 copies reserves one 1 KiB slot instead of eight. Overlapping snapshots still get separate slots; a loop still keeps a single entry alloca. Covered by TestLowerMultiElementArrayCopyStackReuse.
Sub-4KiB aggregate snapshots each got a dedicated entry alloca, so mutually exclusive branches reserved the sum of every copy. Keep the loop-safe entry allocation, share one slot across non-overlapping live ranges of the same type, and mark occupants with llvm.lifetime.start/end. An 8-way switch of [256]uint32 copies now reserves 1KiB instead of 8KiB. Overlapping snapshots still get separate slots.
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
2 participants
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.
Depends on #2690 (host 4KiB aggregate copy pass). This PR adds cmd/compile-style handling for multi-element arrays larger than the CanSSA size limit.
Summary
cmd/compile
CanSSAnever represents arrays with more than one element as SSA values once they exceed 4 pointer words (32 bytes on 64-bit). Copies stay in memory (OpMove/LoweredMoveLoop). LLGo emitted first-classload [N x T]/store [N x T], and LLVMdefault<Os>then scalarized them (SLP + ISel).crypto/internal/fips140/mldsais the sharp case:ringElement/nttElementare[256]uint32(1KiB), under the 4KiB copy threshold.nttstarts with:After C ABI lowering, this PR rewrites those array copies to
memmove, same pass as #2690, gated onsize > 4*PtrSize. Smaller arrays such as[2]int64stay first-class so they can remain in registers. The frontend safepoint predicate uses the sameShouldSnapshotAggregateLoadhelper as the backend.C ABI is unchanged:
MaxImplicitStackVarSize(64KiB) are untouchedLowerLargeAggregates(pre-C-ABI sret) does not rewrite 1KiB array copiesPerformance (
-Osbackend, isolated package)crypto/internal/fips140/mldsacrypto/internal/fips140/mlkemgolang.org/x/tools/go/ast/edgecompress/flatego/ast/edgeand most ofcompress/flateare already covered by the 4KiB copy pass in #2690. This PR is the remaining 1KiB polynomial copies inmldsa/mlkem.Test plan
go test ./internal/abi -count=1 -run 'ShouldLower|MultiElement|AggregateCopies|LargeAggregateThreshold'go test ./cl -count=1 -run TestCompileLargeSnapshotGCRootsgo test ./internal/cabi -count=1[256 x i32]and[5 x i64]→ memmove;[2 x i64]/[1 x i64]unchanged; call-argument arrays unchangedAllocUGC roots