cl, ssa: fix zero-sized global init and fold composite literal alloc stores - #2372
cl, ssa: fix zero-sized global init and fold composite literal alloc stores#2372cpunion wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Review: static-init composite literal folding
This PR extends static-init folding to composite literals reached through pointer indirection (&Alloc + UnOp deref), via a new recursive SSA traversal collectAllocStores, plus a moduleZeroName sentinel guard in ssa/decl.go. The overall structure closely mirrors the careful single-use validation of the existing staticSliceInitOf path, and the sentinel guard correctly prevents clobbering the shared zero-sized initializer.
One correctness gap is worth addressing before merge (see inline): the *ssa.UnOp deref branch suppresses a whole-aggregate load without validating that the loaded value is consumed only by the folded store. Verified findings:
- Suppression is driven by
p.staticInitInstrs;compileInstr(cl/compile.go:1761) returns early for suppressed instructions, and a later live consumer routes throughcompileInstrOrValue(asValue=true)→log.Panicln("unreachable:", iv)(cl/compile.go:1309).
Test coverage: collectAllocStores / handleStoreVal / staticInitStorePathToAlloc are not exercised by any new test. Existing static-init tests cover only direct FieldAddr/IndexAddr-to-global stores, not the &Alloc + UnOp deref shape this PR targets. Please add an IR-level test that folds the pointer-indirection form and asserts no double-emission, plus a negative test for the multi-consumer case described inline.
Performance and the sentinel guard were reviewed and found sound; the visited map bounds recursion and array materialization stays capped by maxStaticInitArrayElements.
| case *ssa.UnOp: | ||
| if ref.Op != token.MUL { | ||
| return false | ||
| } | ||
| *instrs = append(*instrs, ref) |
There was a problem hiding this comment.
[P1] UnOp deref branch suppresses a load without validating its other uses
This branch accepts any *alloc load and appends it to instrs (marking it static-init-consumed) without checking its referrers. Every other branch here validates single-use first: FieldAddr/IndexAddr require len(refs) == 1 and that the sole referrer is the matching Store; the slice path (staticSliceInitOf) enforces the same via nonDebugReferrers.
If the same alloc's loaded value also flows to a non-static consumer (e.g. a second store, or a function argument), the alloc's stores get folded into a constant while the load is suppressed — but the surviving runtime instruction still references it. Suppressed instructions are skipped in compileInstr (cl/compile.go:1761) and never populate bvals; a later live consumer then hits log.Panicln("unreachable:", iv) (cl/compile.go:1309), or reads uninitialized storage.
Recommend mirroring the slice/field paths: reject unless nonDebugReferrers(ref) has no user other than the driving global store. A negative test for the multi-consumer case would lock this down.
|
|
||
| // Init initializes the global variable with the given value. | ||
| func (g Global) Init(v Expr) { | ||
| if g.impl.Name() == moduleZeroName { |
There was a problem hiding this comment.
[P3] Document why Init/InitNil no-op for the zero-sized sentinel
Init and InitNil now silently no-op when the global is the shared moduleZeroName sentinel. The rationale (the sentinel is LinkOnceODR and shares its ConstNull initializer across all zero-sized globals, so a per-variable SetInitializer would clobber it for every other zero-sized global) is not obvious from the guard alone. A one-line comment would make the silent behavior discoverable, consistent with the prose already in doNewVarEx. The Init doc comment ("initializes the global variable with the given value") is also now slightly inaccurate for this case, and InitNil has no doc comment where its sibling does.
c091a40 to
335c406
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…indirection tests
|
Closing in favor of #2371 which consolidates the dependency bump, loader refactoring, and static-init composite literal folding optimization into a single cohesive PR. |
Summary
Zero-Sized Global Variable Initializer Guard (
ssa/decl.go):internal/poll) from overwriting the module's shared sentinel@"__llgo.moduleZeroSizedAlloc$"with non-i8initializers during package global initialization, avoiding LLVM module verification errors.Recursive Composite Literal Static Folding (
cl/static_init.go):init()stack allocation,memset, and element-wise store overhead.appendStaticInitPath) to prevent slice-aliasing hazards during DAG path collection.Unit Tests (
cl/rewrite_internal_test.go):