cl, ssa: fold composite literal stores, support struct slice static inits, and add LTO baseline benchmarks - #2375
Conversation
3c59322 to
72e03db
Compare
72e03db to
d565df7
Compare
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Solid extension of the static-init folding pass to nested composite literals reached through pointer indirection (*(&localAlloc)), plus a correct fix for the zero-sized-alloc sentinel and useful -lto=full benchmark variants.
Verified sound:
- The recursive SSA traversal (
collectAllocStores/collectAddrStores/handleStoreVal) is guarded well:visitedprevents cyclic re-entry (set before recursing), single-referrer/aliasing checks reject shared allocs,appendStaticInitPathdefensively copies to avoid path-slice aliasing across sibling branches, andstaticInitNode.addrejects conflicting value-vs-children assignments. - Traversal is effectively linear in the relevant SSA subgraph; index/field range and array-size caps are enforced.
- The
ssa/decl.goguard is correct: for zero-sized globalsdoNewVarExreturns aGlobalaliased to the sharedmoduleZeroNamesentinel, soInit/InitNilmust not callSetInitializeron it (the sentinel already has a null initializer underLinkOnceODRLinkage). - Doc comments on the new helpers accurately describe behavior, and
benchmark/baseline/README.mdmatches the workloads inmain.go.
Note on the Slice case in collectAllocStores: it records nothing into instrs/out, which is fine because its only caller (staticSliceInitOf) pre-seeds {slice, store} into instrs, and nested slices inside structs are rejected upstream (handleStoreVal only accepts *ssa.Const and *ssa.UnOp(MUL)). No data is dropped there.
Findings below are minor robustness/hardening notes; none block merge.
|
|
||
| // 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.
[P2] Sentinel guard relies on name-string comparison
Init/InitNil detect the shared zero-sized-alloc sentinel via g.impl.Name() == moduleZeroName. This is correct today, but a pointer/identity comparison against the cached sentinel value (or a boolean flag on aGlobal) would be more robust than a name-string match, since it does not depend on LLVM never renaming the symbol. Functionally the change is correct — the sentinel is created once with a null initializer under LinkOnceODRLinkage, so skipping re-initialization is the right behavior.
| git fetch https://github.com/${{ github.event.pull_request.base.repo.full_name }}.git ${{ github.event.pull_request.base.ref }} | ||
| base_sha=$(git merge-base FETCH_HEAD ${{ github.event.pull_request.head.sha }}) | ||
| echo "sha=$base_sha" >> "$GITHUB_OUTPUT" | ||
| echo "Computed pull request merge-base: $base_sha (head: ${{ github.event.pull_request.head.sha }})" |
There was a problem hiding this comment.
[P3] Untrusted event fields interpolated into a run step
The Determine pull request merge-base step expands ${{ github.event.pull_request.* }} directly into a shell run: block — the canonical GitHub Actions script-injection shape. Severity is low here: the trigger is pull_request (read-only GITHUB_TOKEN, no secrets), head.sha is a hex SHA, and base.repo.full_name/base.ref are base-side (maintainer-controlled for fork PRs). Recommend passing these through env: and referencing quoted shell variables (e.g. "$BASE_REF", "$HEAD_SHA") so the workflow stays safe against future refactors (e.g. to pull_request_target).
| if !ok || elemStore.Val != ref { | ||
| return false | ||
| } | ||
| case *ssa.UnOp: |
There was a problem hiding this comment.
[P3] UnOp load suppressed without checking its consumer
In the *ssa.UnOp branch of collectAllocStores, the load (*alloc) is required to have exactly one referrer and is then added to *instrs (suppressed), but that single referrer is never inspected or recorded. For the currently reachable patterns this is safe (the consuming store is already tracked/suppressed by the caller), but the function is recursive and reused, so suppressing the load while leaving a live consumer would create an SSA reference to a never-emitted value. Consider verifying unopRefs[0] is (or will be) suppressed, or rejecting the case, to make the invariant explicit rather than implicit.
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
Summary
Folds package-level composite literal initializers and struct/composite slice initializers into LLVM static constants, removing runtime allocation loops in
init()functions, and adds baseline LTO workload measurements.Key Changes
Composite Literal & Struct Slice Static Folding (
cl/static_init.go):staticSliceInitOfandcollectAllocStoreswith recursivecollectAddrStoressupport to handle arbitrary nested struct field address projections and multi-element stores.[]Info{...}) directly into static LLVM constant slice data (p.pkg.ConstSlice), eliminating runtimeAllocZand store loops from packageinit()functions.init().Zero-Sized Global Init Guard (
ssa/decl.go):[0]int{}) emit valid constant zeroinitializers.Baseline Benchmark Measurements (
.github/workflows/benchmark.yml,benchmark/baseline/):-lto=fullworkload measurements (cprintf_lto,println_lto,fmtprintf_lto).merge-basecomputation for exact baseline comparisons.Test Coverage (
cl/rewrite_internal_test.go):TestStaticGlobalStructSliceLiteralInitand comprehensive literal folding test suites.