all: bump golang.org/x/tools to v0.49.0, fold composite literals, and add LTO baseline benchmarks - #2371
all: bump golang.org/x/tools to v0.49.0, fold composite literals, and add LTO baseline benchmarks#2371cpunion wants to merge 3 commits into
Conversation
2004fb0 to
fd12fbe
Compare
There was a problem hiding this comment.
Review: bump goplus/mod to 0.22.0 + loader rewrite
Solid change overall. The dependency bump (goplus/mod 0.22.0, golang.org/x/{mod,sys,tools,sync}) forced dropping the go:linkname hooks into x/tools' internal loader symbols, and the internal/packages/load.go rewrite replaces them with a self-contained implementation over the public packages.Load + packages.Visit. go.sum hashes verify against the checksum database. The ssa/decl.go moduleZeroName guard and the new pointer/nested-alloc handling in cl/static_init.go are reasonable additions.
One issue worth fixing before merge — a slice-aliasing hazard in the new collectAllocStores that can silently miscompile static initializers (inline comment below).
Secondary notes (non-blocking):
internal/packages/load.go— serial typecheck. The old loader parsed/typechecked dependencies in parallel goroutines; the newpackages.Visitpost-order loop runstypecheckPackageserially. This is an acceptable trade-off (correctness/simplicity, and post-order guarantees importedTypesare complete before dependents), but it's a real compile-time cost on large dependency graphs — worth noting as known.internal/packages/load.gobehavior parity. The rewrite dropsIgnoreFuncBodies, theFakeImportCimport "C" ignoreddiagnostic, and therequestedModefield-zeroing that trimmed returnedPackagefields to the requested mode. Both in-repo callers use the fullloadSyntaxmode so they're unaffected, butLoadEx/LoadExWithGoVersionare exported — returned fields are no longer trimmed to the requested mode. Consider documenting this in the doc comment.internal/packages/load.go:262—targetCompilerAndArch(pkg *Package)never uses itspkgparameter; drop it.internal/packages/load.go:156— doc-comment nit. TheLoadExcomment (inherited from upstreampackages.Load) refers to "the PrintErrors function is provided" and "Load returns an error", but this package re-exports neitherPrintErrorsnor aLoadsymbol. Minor wording drift.
| if !ok || elemStore.Addr != ref { | ||
| return false | ||
| } | ||
| if !handleStoreVal(elemStore, append(basePath, subPath...), out, instrs, visited) { |
There was a problem hiding this comment.
[P1] Slice-aliasing corrupts static-init paths in collectAllocStores
Slice-aliasing hazard: append(basePath, subPath...) results are stored long-term.
handleStoreVal stores the passed path slice directly into out[].path (staticInitStore.path), which is consumed much later in buildStaticGlobalInit → staticInitNode.add. But append(basePath, subPath...) reuses basePath's backing array whenever cap(basePath) > len(basePath). When an alloc has two or more FieldAddr/IndexAddr referrers (a struct with ≥2 fields, an array with ≥2 indices), the second iteration's append overwrites the tail of the path recorded for the first sibling — the retained path in out is silently corrupted.
basePath carries spare capacity in the common case: the nested-alloc recursion at line 283 passes fullPath (itself an append result, so cap > len), and the top-level call also passes a built-up path when the deref store targets a sub-path of the global. The result is a silent miscompilation (wrong field/element gets the value), not a crash.
Fix by copying per element, e.g.:
full := append(append([]staticInitPathElem(nil), basePath...), subPath...)
if !handleStoreVal(elemStore, full, out, instrs, visited) {(same at line 253). staticInitStorePath and staticInitStorePathToAlloc build fresh slices along a single linear recursion, so they're safe.
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
All review feedback addressed in the latest commit:
|
894e64a to
b87e8db
Compare
b87e8db to
4876c52
Compare
4876c52 to
ab233da
Compare
…res, and update test fixtures for x/tools v0.49.0
ab233da to
4fc79b7
Compare
Summary
Bumps
github.com/goplus/modtov0.22.0(which bringsgolang.org/x/toolsv0.49.0,golang.org/x/modv0.40.0,golang.org/x/sysv0.47.0, andgolang.org/x/syncv0.22.0), adapts compiler lowering togo/ssav0.49.0, enhances static initializer folding for package-level composite literals, and adds LTO measurements and PR merge-base baseline tracking to the benchmark suite.Key Changes
Dependency Upgrade:
github.com/goplus/modfromv0.21.2tov0.22.0golang.org/x/toolstov0.49.0golang.org/x/modtov0.40.0,golang.org/x/systov0.47.0,golang.org/x/synctov0.22.0Composite Literal Static Folding (
cl/static_init.go):x/tools v0.49.0,go/ssalowers composite literals ininit()using temporary local stack variables (bottom-up*ssa.Alloc+*ssa.Store+*ssa.UnOp(token.MUL)) rather than direct global field stores.cl/static_init.go(collectAllocStores) to recursively trace and fold these local temporary alloc stores into static LLVM global constants (avoiding runtimeinit()stack allocation,memset, and element-wise store instructions, saving ~52.6 KB of binary bloat).appendStaticInitPathto allocate isolated slice paths, preventing slice-aliasing hazards during DAG path collection.*ssa.UnOp(MUL)to ensure dereferenced values are consumed solely by the folded store before instruction suppression.Zero-Sized Global Sentinel Guard (
ssa/decl.go):internal/poll) from overwriting the module's shared sentinel@"__llgo.moduleZeroSizedAlloc$"with non-i8initializers during package initialization, avoiding LLVM module verification panics.Benchmark Enhancements (
.github/workflows/benchmark.yml,benchmark/baseline/):git merge-basecommit between base and head, eliminating base drift from concurrent main branch merges.-lto=fullworkload measurements forcprintf_lto,println_lto, andfmtprintf_lto.Test Fixtures & Coverage (
cl/rewrite_internal_test.go,cl/_test*):cl/_testgo/andcl/_testrt/forx/tools v0.49.0lowering (explicitstore zeroinitializerfollowingAllocZ, stack slot adjustments).TestStaticGlobalPointerIndirectionLiteralInitandTestCollectAllocStoresFromSSAincl/rewrite_internal_test.gocovering nested composite literals, pointer indirections, and edge cases.Verification
go test -v ./cl -run 'TestStaticGlobal|TestBlankField|TestCollectAllocStores|TestRunAndTestFromTestrt/qsort'(PASS)go test -v ./benchmark/baseline(PASS)go test -v ./internal/packages(PASS)bash test/buildcache/test.sh(18/18 PASS)