cl: fold slices of struct and array values into static initializers - #2378
cl: fold slices of struct and array values into static initializers#2378cpunion wants to merge 11 commits into
Conversation
…res, and harden static init helpers
There was a problem hiding this comment.
Review: fold struct/slice on static init
This PR extends static-init folding to trace constant stores through local stack allocs (nested structs/slices/pointers) and adds -lto=full benchmark workloads. The recursive SSA traversal is generally well-guarded — visited prevents cyclic pointer graphs, appendStaticInitPath correctly allocates fresh backing slices to avoid append-aliasing, staticInitNode.add conservatively aborts on any path collision, and index/field bounds are validated at multiple layers. The ssa/decl.go moduleZeroName guards are correct and well-documented, and the benchmark workflow uses pull_request (read-only token) with persist-credentials: false, so the untrusted-fork surface is contained.
I found one correctness regression that should be fixed before merge (inline). A few lower-priority notes are below.
Lower-priority observations (no inline):
-
Redundant load suppression (
cl/static_init.go): the top-levelunopis appended tocandidate.instrs(~line 155) and is also suppressed insidecollectAllocStoresvia theUnOpcase (~line 246), since it is one of the alloc's referrers. Harmless becausestaticInitInstrsis a map, but the overlapping ownership of suppression between caller and callee is worth consolidating for clarity. -
UnOpcase suppresses any single-referrer load without checking its consumer (cl/static_init.go~line 238-246): the internalUnOpbranch only verifies the load has exactly one referrer before suppressing it, unlike the entry points (~lines 147, 333) which assertunopRefs[0] == store. Given the current single-referrer checks this appears safe, but confirming the consumer is part of the fold would make the suppression contract more robust. Low confidence — flagging for author awareness. -
O(depth²) path reconstruction (
cl/static_init.go,staticInitStorePathToAlloc/appendStaticInitPath): each recursion level re-allocates and copies the accumulated prefix, giving O(d²) copies per store. Impact is bounded by shallow struct/array nesting depth and the 65536-element array cap, so this is minor — noting only as a possible cleanup (thread the prefix down, or size once at the leaf). -
staticInitPathElemis a single-field struct — a bareint/named int type would simplify the many[]staticInitPathElem{{index: ...}}constructions if no further fields are planned. Style only.
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…and add unit tests
affefb0 to
d901863
Compare
|
Addressed the review follow-ups in
Added real-SSA regressions for extra loads, shared slice backing, bounded slices, dynamic fields, nested aggregates, and malformed paths. The targeted static-init tests, zero-sized alias test, benchmark baseline tests, and repository formatting check all pass locally. |
d901863 to
c2fb0ee
Compare
c2fb0ee to
1efd309
Compare
Note
Depends on #2376: this PR extends the alloc-backed global-initializer folding introduced there.
Summary
Extends static initialization to eligible package-level slice literals whose backing arrays contain struct or array values, including nested combinations of those types. Constant elements are emitted into a writable package global and the slice header is installed as the destination global initializer, removing the corresponding runtime allocation and store sequence from package initialization.
Dynamic, shared, bounded, malformed, or otherwise ambiguous SSA shapes continue to use normal runtime initialization.
Implementation
Recursive store collection (
cl/static_init.go)Static slice construction
staticInitNode.p.pkg.ConstSliceto create a slice value backed by writable static storage, preserving Go slice mutability.Regression coverage (
cl/rewrite_internal_test.go)Measured Impact
The baseline workflow reports these
fmtprintfexecutable file-size reductions against the main merge base:All CI and Codecov checks pass.