cl: support static initializer folding for struct and composite slices - #2374
cl: support static initializer folding for struct and composite slices#2374cpunion wants to merge 5 commits into
Conversation
…res, and update test fixtures for x/tools v0.49.0
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fold struct/slice static inits
Solid, well-structured change with unusually thorough test coverage (cl/rewrite_internal_test.go exercises cycle detection, out-of-range children, blank fields, executable referrers, and zero-sized fallbacks). The recursive tracing is correctly bounded by the visited set and single-referrer checks (linear in traversed instructions), and large-array folding is capped by maxStaticInitArrayElements. Dependency bumps in go.mod/go.sum are clean version updates with no substitutions. Doc comments and the benchmark README changes accurately match the code/config.
Two soundness concerns in the new SSA tracing in cl/static_init.go are worth a look before merge (both flagged inline). They are non-blocking observations — the invariants happen to hold for the current entry points, but the helper functions don't enforce them internally.
Minor / nits:
staticInitStorePathToAlloc(~line 337-355) andstaticInitStorePath(~line 463-481) build paths viaappendon the recursively-returned slice. This is the same slice-aliasing hazard thatappendStaticInitPathwas introduced to avoid. It's safe today because these call sites are always single-level, but a short comment (or reusing the non-aliasing helper) would prevent a future multi-level caller from hitting aliasing.ssa/decl.goGlobal.Init/InitNil: themoduleZeroNameguard silently no-ops. Correct, but a one-line comment on why (mirroring the good comment indoNewVarEx) would help future readers.
| if !ok || elemStore.Val != ref { | ||
| return false | ||
| } | ||
| case *ssa.UnOp: |
There was a problem hiding this comment.
Load consumer not validated — potential unsound folding.
The *ssa.UnOp (load) branch appends the load to instrs (suppressing it) but only checks that the load has exactly one referrer — it never inspects what that referrer is. The callers (collectStaticGlobalInits and handleStoreVal) validate that the specific load they originate from feeds the expected store, but an alloc can have additional load referrers.
If the traced alloc is loaded a second time and that value flows into a call or another store, this branch accepts it (len(unopRefs) == 1 passes), the load and the alloc's stores get suppressed, yet the extra consumer is left referencing a value whose producing load is no longer emitted (and reads an alloc that is no longer initialized at runtime). That is a potential miscompilation.
Consider validating that unopRefs[0] is itself part of the folded/suppressed set (mirroring the elemStore.Addr == ref checks in the FieldAddr/IndexAddr cases), or returning false for unrecognized consumers. TestStaticSliceInitRejectsExecutableReferrers only covers the top-level alloc, not a nested/aliased load.
| } | ||
| for _, ref := range refs { | ||
| switch ref := ref.(type) { | ||
| case *ssa.Slice: |
There was a problem hiding this comment.
*ssa.Slice branch validates but neither records nor suppresses its instructions.
This *ssa.Slice branch validates the shape (single referrer that is a store consuming the slice) but — unlike the FieldAddr/IndexAddr/Store cases — it neither appends ref/elemStore to *instrs nor checks elemStore.Addr, and it collects no values.
When collectAllocStores is entered from staticSliceInitOf, the slice+store are pre-seeded into instrs separately, so this is fine there. But the function is also reachable for nested allocs via handleStoreVal -> collectAllocStores(innerAlloc, ...). If such a nested alloc has a *ssa.Slice referrer, this branch returns success while leaving the slice and its store un-suppressed, risking inconsistent/duplicate initialization.
The correctness of this branch is coupled to the specific caller rather than self-contained. Recommend either recording/suppressing the slice's store here (and validating its target) or explicitly rejecting the Slice shape outside the dedicated slice entry point, plus a clarifying comment.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
|
Closing in favor of restructured PRs. |
1 participant
Note
Depends on #2371: This PR builds on top of #2371 (
all: bump golang.org/x/tools to v0.49.0, fold composite literals, and add LTO baseline benchmarks).Summary
Extends
cl/static_init.go's static initializer folding engine from scalar-only slices to arbitrary struct and composite literal slices (such as[]Struct{...}).Key Changes
Struct & Composite Slice Static Folding (
cl/static_init.go):staticSliceInitOfto collect and resolve element stores recursively throughcollectAllocStores.buildStaticSliceInitto reconstruct per-element AST/SSA trees viastaticInitNode.add()and evaluate them into LLVM struct constants (p.pkg.ConstSlice(...)), eliminating runtime element assignment loops in packageinit()functions.init().Test Coverage (
cl/rewrite_internal_test.go):TestStaticGlobalStructSliceLiteralInitverifying that package-level struct slice variables (e.g.var All = []Info{...}) compile into static global constants (@"staticinit.All$data") without emittingruntime.AllocZor runtime initializer stores.Measured Impact
internal/godebugs.init: Initializer function size reduced from 3,780 bytes to 24 bytes (99.4% reduction, -3.75 KB text).__text, enabling linker-dead_stripand-deadcodedropto discard unreferenced slices cleanly.