From cc258e6785060345a497d44b6073c91b81e32576 Mon Sep 17 00:00:00 2001 From: David Chase Date: Wed, 22 May 2024 14:00:43 -0700 Subject: [PATCH] cmd/compile: add "deadlocals" pass to remove unused locals This CL adds a "deadlocals" pass, which runs after inlining and before escape analysis, to prune any unneeded local variables and assignments. In particular, this helps avoid unnecessary Addrtaken markings from unreachable closures. Deadlocals is sensitive to "_ = ..." as a signal of explicit use for testing. This signal occurs only if the entire left-hand-side is "_" targets; if it is `_, ok := someInlinedFunc(args)` then the first return value is eligible for dead code elimination. Use this (`_ = x`) to fix tests broken by deadlocals elimination. Includes a test, based on one of the tests that required modification. Matthew Dempsky wrote this, changing ownership to allow rebases, commits, tweaks. Fixes #65158. Old-Change-Id: I723fb69ccd7baadaae04d415702ce6c8901eaf4e Change-Id: I1f25f4293b19527f305c18c3680b214237a7714c Reviewed-on: https://go-review.googlesource.com/c/go/+/600498 Reviewed-by: Keith Randall LUCI-TryBot-Result: Go LUCI Reviewed-by: Keith Randall Auto-Submit: David Chase Commit-Queue: David Chase --- src/cmd/compile/internal/base/debug.go | 1 + .../compile/internal/deadlocals/deadlocals.go | 187 ++++++++++++++++++ src/cmd/compile/internal/gc/main.go | 3 + src/runtime/race/testdata/mop_test.go | 8 +- test/closure3.dir/main.go | 23 +++ test/fixedbugs/issue54159.go | 1 + test/inline.go | 8 +- test/newinline.go | 8 +- 8 files changed, 235 insertions(+), 4 deletions(-) create mode 100644 src/cmd/compile/internal/deadlocals/deadlocals.go diff --git a/src/cmd/compile/internal/base/debug.go b/src/cmd/compile/internal/base/debug.go index c1b62f27ca..f53a32f8fa 100644 --- a/src/cmd/compile/internal/base/debug.go +++ b/src/cmd/compile/internal/base/debug.go @@ -48,6 +48,7 @@ type DebugFlags struct { MergeLocalsTrace int `help:"trace debug output for locals merging"` MergeLocalsHTrace int `help:"hash-selected trace debug output for locals merging"` Nil int `help:"print information about nil checks"` + NoDeadLocals int `help:"disable deadlocals pass" concurrent:"ok"` NoOpenDefer int `help:"disable open-coded defers" concurrent:"ok"` NoRefName int `help:"do not include referenced symbol names in object file" concurrent:"ok"` PCTab string `help:"print named pc-value table\nOne of: pctospadj, pctofile, pctoline, pctoinline, pctopcdata"` diff --git a/src/cmd/compile/internal/deadlocals/deadlocals.go b/src/cmd/compile/internal/deadlocals/deadlocals.go new file mode 100644 index 0000000000..f40ca71970 --- /dev/null +++ b/src/cmd/compile/internal/deadlocals/deadlocals.go @@ -0,0 +1,187 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// The deadlocals pass removes assignments to unused local variables. +package deadlocals + +import ( + "cmd/compile/internal/base" + "cmd/compile/internal/ir" + "cmd/compile/internal/types" + "cmd/internal/src" + "fmt" + "go/constant" +) + +// Funcs applies the deadlocals pass to fns. +func Funcs(fns []*ir.Func) { + if base.Flag.N != 0 || base.Debug.NoDeadLocals != 0 { + return + } + + zero := ir.NewBasicLit(base.AutogeneratedPos, types.Types[types.TINT], constant.MakeInt64(0)) + + for _, fn := range fns { + if fn.IsClosure() { + continue + } + + v := newVisitor(fn) + v.nodes(fn.Body) + + for _, assigns := range v.defs { + for _, as := range assigns { + // Kludge for "missing func info" linker panic. + // See also closureInitLSym in inline/inl.go. + if clo, ok := (*as.rhs).(*ir.ClosureExpr); ok && clo.Op() == ir.OCLOSURE { + if !ir.IsTrivialClosure(clo) { + ir.InitLSym(clo.Func, true) + } + } + + *as.lhs = ir.BlankNode + *as.rhs = zero + } + } + } +} + +type visitor struct { + curfn *ir.Func + // defs[name] contains assignments that can be discarded if name can be discarded. + // if defs[name] is defined nil, then name is actually used. + defs map[*ir.Name][]assign + + doNode func(ir.Node) bool +} + +type assign struct { + pos src.XPos + lhs, rhs *ir.Node +} + +func newVisitor(fn *ir.Func) *visitor { + v := &visitor{ + curfn: fn, + defs: make(map[*ir.Name][]assign), + } + v.doNode = func(n ir.Node) bool { + v.node(n) + return false + } + return v +} + +func (v *visitor) node(n ir.Node) { + if n == nil { + return + } + + switch n.Op() { + default: + ir.DoChildrenWithHidden(n, v.doNode) + case ir.OCLOSURE: + n := n.(*ir.ClosureExpr) + v.nodes(n.Init()) + for _, cv := range n.Func.ClosureVars { + v.node(cv) + } + v.nodes(n.Func.Body) + + case ir.ODCL: + // ignore + case ir.ONAME: + n := n.(*ir.Name) + n = n.Canonical() + if isLocal(n, false) { + // Force any lazy definitions. + s := v.defs[n] + v.defs[n] = nil + + for _, as := range s { + // do the visit that was skipped in v.assign when as was appended to v.defs[n] + v.node(*as.rhs) + } + } + + case ir.OAS: + n := n.(*ir.AssignStmt) + v.assign(n.Pos(), &n.X, &n.Y, false) + case ir.OAS2: + n := n.(*ir.AssignListStmt) + + // If all LHS vars are blank, treat them as intentional + // uses of corresponding RHS vars. If any are non-blank + // then any blanks are discards. + hasNonBlank := false + for i := range n.Lhs { + if !ir.IsBlank(n.Lhs[i]) { + hasNonBlank = true + break + } + } + for i := range n.Lhs { + v.assign(n.Pos(), &n.Lhs[i], &n.Rhs[i], hasNonBlank) + } + } +} + +func (v *visitor) nodes(list ir.Nodes) { + for _, n := range list { + v.node(n) + } +} + +func hasEffects(n ir.Node) bool { + if n == nil { + return false + } + if len(n.Init()) != 0 { + return true + } + + switch n.Op() { + // TODO(mdempsky): More. + case ir.ONAME, ir.OLITERAL, ir.ONIL, ir.OCLOSURE: + return false + } + return true +} + +func (v *visitor) assign(pos src.XPos, lhs, rhs *ir.Node, blankIsNotUse bool) { + name, ok := (*lhs).(*ir.Name) + if !ok { + v.node(*lhs) // XXX: Interpret as variable, not value. + v.node(*rhs) + return + } + name = name.Canonical() + + if isLocal(name, blankIsNotUse) && !hasEffects(*rhs) { + if s, ok := v.defs[name]; !ok || s != nil { + // !ok || s != nil is FALSE if previously "v.defs[name] = nil" -- that marks a use. + v.defs[name] = append(s, assign{pos, lhs, rhs}) + return // don't visit rhs unless that node ends up live, later. + } + } + + v.node(*rhs) +} + +func isLocal(n *ir.Name, blankIsNotUse bool) bool { + if ir.IsBlank(n) { + // Treat single assignments as intentional use (false), anything else is a discard (true). + return blankIsNotUse + } + + switch n.Class { + case ir.PAUTO, ir.PPARAM: + return true + case ir.PPARAMOUT: + return false + case ir.PEXTERN, ir.PFUNC: + return false + } + panic(fmt.Sprintf("unexpected Class: %+v", n)) +} diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go index f68cf4deaf..174c609e44 100644 --- a/src/cmd/compile/internal/gc/main.go +++ b/src/cmd/compile/internal/gc/main.go @@ -9,6 +9,7 @@ import ( "bytes" "cmd/compile/internal/base" "cmd/compile/internal/coverage" + "cmd/compile/internal/deadlocals" "cmd/compile/internal/dwarfgen" "cmd/compile/internal/escape" "cmd/compile/internal/inline" @@ -247,6 +248,8 @@ func Main(archInit func(*ssagen.ArchInfo)) { // and doesn't benefit from dead-coding or inlining. symABIs.GenABIWrappers() + deadlocals.Funcs(typecheck.Target.Funcs) + // Escape analysis. // Required for moving heap allocations onto stack, // which in turn is required by the closure implementation, diff --git a/src/runtime/race/testdata/mop_test.go b/src/runtime/race/testdata/mop_test.go index 6b1069fcca..0d7d879d83 100644 --- a/src/runtime/race/testdata/mop_test.go +++ b/src/runtime/race/testdata/mop_test.go @@ -612,6 +612,8 @@ func TestNoRaceEnoughRegisters(t *testing.T) { } // emptyFunc should not be inlined. +// +//go:noinline func emptyFunc(x int) { if false { fmt.Println(x) @@ -1176,7 +1178,7 @@ func TestNoRaceHeapReallocation(t *testing.T) { // others. const n = 2 done := make(chan bool, n) - empty := func(p *int) {} + empty := func(p *int) { _ = p } for i := 0; i < n; i++ { ms := i go func() { @@ -1417,7 +1419,7 @@ func TestRaceInterCall2(t *testing.T) { func TestRaceFuncCall(t *testing.T) { c := make(chan bool, 1) - f := func(x, y int) {} + f := func(x, y int) { _ = y } x, y := 0, 0 go func() { y = 42 @@ -1804,6 +1806,7 @@ func TestRaceAsFunc2(t *testing.T) { x := 0 go func() { func(x int) { + _ = x }(x) c <- true }() @@ -1817,6 +1820,7 @@ func TestRaceAsFunc3(t *testing.T) { x := 0 go func() { func(x int) { + _ = x mu.Lock() }(x) // Read of x must be outside of the mutex. mu.Unlock() diff --git a/test/closure3.dir/main.go b/test/closure3.dir/main.go index e3981a5161..7bed78c308 100644 --- a/test/closure3.dir/main.go +++ b/test/closure3.dir/main.go @@ -20,6 +20,7 @@ func main() { if x := func() int { // ERROR "can inline main.func2" "func literal does not escape" return 1 }; x() != 1 { // ERROR "inlining call to main.func2" + _ = x // prevent simple deadcode elimination after inlining ppanic("x() != 1") } } @@ -33,6 +34,7 @@ func main() { if y := func(x int) int { // ERROR "can inline main.func4" "func literal does not escape" return x + 2 }; y(40) != 42 { // ERROR "inlining call to main.func4" + _ = y // prevent simple deadcode elimination after inlining ppanic("y(40) != 42") } } @@ -181,6 +183,7 @@ func main() { if y := func() int { // ERROR "can inline main.func21" "func literal does not escape" return x }; y() != 42 { // ERROR "inlining call to main.func21" + _ = y // prevent simple deadcode elimination after inlining ppanic("y() != 42") } } @@ -199,6 +202,7 @@ func main() { return x + y }() // ERROR "inlining call to main.func23.1" }; z(1) != 43 { // ERROR "inlining call to main.func23" "inlining call to main.main.func23.func31" + _ = z // prevent simple deadcode elimination after inlining ppanic("z(1) != 43") } } @@ -287,6 +291,25 @@ func main() { } } +//go:noinline +func notmain() { + { + // This duplicates the first block in main, but without the "_ = x" for closure x. + // This allows dead code elimination of x before escape analysis, + // thus "func literal does not escape" should not appear. + if x := func() int { // ERROR "can inline notmain.func1" + return 1 + }(); x != 1 { // ERROR "inlining call to notmain.func1" + ppanic("x != 1") + } + if x := func() int { // ERROR "can inline notmain.func2" + return 1 + }; x() != 1 { // ERROR "inlining call to notmain.func2" + ppanic("x() != 1") + } + } +} + //go:noinline func ppanic(s string) { // ERROR "leaking param: s" panic(s) // ERROR "s escapes to heap" diff --git a/test/fixedbugs/issue54159.go b/test/fixedbugs/issue54159.go index 8ef0e68483..0f607b38e1 100644 --- a/test/fixedbugs/issue54159.go +++ b/test/fixedbugs/issue54159.go @@ -11,6 +11,7 @@ func run() { // ERROR "cannot inline run: recursive" g() // ERROR "inlining call to g" } f() // ERROR "inlining call to run.func1" "inlining call to g" + _ = f run() } diff --git a/test/inline.go b/test/inline.go index fd14f25983..4714c795c2 100644 --- a/test/inline.go +++ b/test/inline.go @@ -73,6 +73,7 @@ func l(x, y int) (int, int, error) { // ERROR "can inline l" f := e f(nil) // ERROR "inlining call to l.func1" } + _ = e // prevent simple deadcode elimination after inlining return y, x, nil } @@ -109,6 +110,7 @@ func p() int { // ERROR "can inline p" func q(x int) int { // ERROR "can inline q" foo := func() int { return x * 2 } // ERROR "can inline q.func1" "func literal does not escape" + _ = foo // prevent simple deadcode elimination after inlining return foo() // ERROR "inlining call to q.func1" } @@ -121,6 +123,8 @@ func r(z int) int { return 2*y + x*z }(x) // ERROR "inlining call to r.func2.1" } + _, _ = foo, bar // prevent simple deadcode elimination after inlining + return foo(42) + bar(42) // ERROR "inlining call to r.func1" "inlining call to r.func2" "inlining call to r.r.func2.func3" } @@ -128,7 +132,8 @@ func s0(x int) int { // ERROR "can inline s0" foo := func() { // ERROR "can inline s0.func1" "func literal does not escape" x = x + 1 } - foo() // ERROR "inlining call to s0.func1" + foo() // ERROR "inlining call to s0.func1" + _ = foo // prevent simple deadcode elimination after inlining return x } @@ -137,6 +142,7 @@ func s1(x int) int { // ERROR "can inline s1" return x } x = x + 1 + _ = foo // prevent simple deadcode elimination after inlining return foo() // ERROR "inlining call to s1.func1" } diff --git a/test/newinline.go b/test/newinline.go index 69f1310ab2..a7288691cd 100644 --- a/test/newinline.go +++ b/test/newinline.go @@ -73,6 +73,7 @@ func l(x, y int) (int, int, error) { // ERROR "can inline l" f := e f(nil) // ERROR "inlining call to l.func1" } + _ = e // prevent simple deadcode elimination return y, x, nil } @@ -109,6 +110,7 @@ func p() int { // ERROR "can inline p" func q(x int) int { // ERROR "can inline q" foo := func() int { return x * 2 } // ERROR "can inline q.func1" "func literal does not escape" + _ = foo // prevent simple deadcode elimination return foo() // ERROR "inlining call to q.func1" } @@ -121,6 +123,8 @@ func r(z int) int { // ERROR "can inline r" return 2*y + x*z }(x) // ERROR "inlining call to r.func2.1" } + _ = foo // prevent simple deadcode elimination + _ = bar // prevent simple deadcode elimination return foo(42) + bar(42) // ERROR "inlining call to r.func1" "inlining call to r.func2" "inlining call to r.r.func2.func3" } @@ -128,7 +132,8 @@ func s0(x int) int { // ERROR "can inline s0" foo := func() { // ERROR "can inline s0.func1" "func literal does not escape" x = x + 1 } - foo() // ERROR "inlining call to s0.func1" + foo() // ERROR "inlining call to s0.func1" + _ = foo // prevent simple deadcode elimination return x } @@ -137,6 +142,7 @@ func s1(x int) int { // ERROR "can inline s1" return x } x = x + 1 + _ = foo // prevent simple deadcode elimination return foo() // ERROR "inlining call to s1.func1" } -- 2.48.1