From 8854be4180fe1fb59c56c645ef8978788eb40170 Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Fri, 14 Apr 2023 13:08:34 +0000 Subject: [PATCH] Revert "cmd/compile: allow more inlining of functions that construct closures" This reverts commit http://go.dev/cl/c/482356. Reason for revert: Reverting this change again, since it is causing additional failures in google-internal testing. Change-Id: I9234946f62e5bb18c2f873a65e8b298d04af0809 Reviewed-on: https://go-review.googlesource.com/c/go/+/484735 Reviewed-by: Florian Zenker Run-TryBot: Than McIntosh Auto-Submit: Than McIntosh Reviewed-by: Than McIntosh TryBot-Result: Gopher Robot Reviewed-by: Cuong Manh Le --- src/cmd/compile/internal/inline/inl.go | 11 +++++----- src/cmd/compile/internal/test/inl_test.go | 22 +++++++++++-------- test/closure3.dir/main.go | 26 ++++++++++------------- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/cmd/compile/internal/inline/inl.go b/src/cmd/compile/internal/inline/inl.go index 9a2df95718..d030a822fc 100644 --- a/src/cmd/compile/internal/inline/inl.go +++ b/src/cmd/compile/internal/inline/inl.go @@ -459,8 +459,6 @@ func (v *hairyVisitor) tooHairy(fn *ir.Func) bool { return false } -// doNode visits n and its children, updates the state in v, and returns true if -// n makes the current function too hairy for inlining. func (v *hairyVisitor) doNode(n ir.Node) bool { if n == nil { return false @@ -592,10 +590,13 @@ func (v *hairyVisitor) doNode(n ir.Node) bool { // TODO(danscales): Maybe make budget proportional to number of closure // variables, e.g.: //v.budget -= int32(len(n.(*ir.ClosureExpr).Func.ClosureVars) * 3) - // TODO(austin): However, if we're able to inline this closure into - // v.curFunc, then we actually pay nothing for the closure captures. We - // should try to account for that if we're going to account for captures. v.budget -= 15 + // Scan body of closure (which DoChildren doesn't automatically + // do) to check for disallowed ops in the body and include the + // body in the budget. + if doList(n.(*ir.ClosureExpr).Func.Body, v.do) { + return true + } case ir.OGO, ir.ODEFER, diff --git a/src/cmd/compile/internal/test/inl_test.go b/src/cmd/compile/internal/test/inl_test.go index 205b746dd8..2a16b21cef 100644 --- a/src/cmd/compile/internal/test/inl_test.go +++ b/src/cmd/compile/internal/test/inl_test.go @@ -180,15 +180,19 @@ func TestIntendedInlining(t *testing.T) { "net": { "(*UDPConn).ReadFromUDP", }, - "sync": { - // Both OnceFunc and its returned closure need to be inlinable so - // that the returned closure can be inlined into the caller of OnceFunc. - "OnceFunc", - "OnceFunc.func2", // The returned closure. - // TODO(austin): It would be good to check OnceValue and OnceValues, - // too, but currently they aren't reported because they have type - // parameters and aren't instantiated in sync. - }, + // These testpoints commented out for now, since CL 479095 + // had to be reverted. We can re-enable this once we roll + // forward with a new version of 479095. + /* + "sync": { + // Both OnceFunc and its returned closure need to be inlinable so + // that the returned closure can be inlined into the caller of OnceFunc. + "OnceFunc", + "OnceFunc.func2", // The returned closure. + // TODO(austin): It would be good to check OnceValue and OnceValues, + // too, but currently they aren't reported because they have type + // parameters and aren't instantiated in sync. + }, */ "sync/atomic": { // (*Bool).CompareAndSwap handled below. "(*Bool).Load", diff --git a/test/closure3.dir/main.go b/test/closure3.dir/main.go index 04a669206e..4d02a4d10e 100644 --- a/test/closure3.dir/main.go +++ b/test/closure3.dir/main.go @@ -232,15 +232,15 @@ func main() { { c := 3 - func() { // ERROR "can inline main.func26" + func() { // ERROR "func literal does not escape" c = 4 - func() { + func() { // ERROR "func literal does not escape" if c != 4 { ppanic("c != 4") } recover() // prevent inlining }() - }() // ERROR "inlining call to main.func26" "func literal does not escape" + }() if c != 4 { ppanic("c != 4") } @@ -248,37 +248,33 @@ func main() { { a := 2 - // This has an unfortunate exponential growth, where as we visit each - // function, we inline the inner closure, and that constructs a new - // function for any closures inside the inner function, and then we - // revisit those. E.g., func34 and func36 are constructed by the inliner. - if r := func(x int) int { // ERROR "can inline main.func27" + if r := func(x int) int { // ERROR "func literal does not escape" b := 3 - return func(y int) int { // ERROR "can inline main.func27.1" "can inline main.func34" + return func(y int) int { // ERROR "can inline main.func27.1" c := 5 - return func(z int) int { // ERROR "can inline main.func27.1.1" "can inline main.func27.(func)?2" "can inline main.func34.1" "can inline main.func36" + return func(z int) int { // ERROR "can inline main.func27.1.1" "can inline main.func27.(func)?2" return a*x + b*y + c*z }(10) // ERROR "inlining call to main.func27.1.1" }(100) // ERROR "inlining call to main.func27.1" "inlining call to main.func27.(func)?2" - }(1000); r != 2350 { // ERROR "inlining call to main.func27" "inlining call to main.func34" "inlining call to main.func36" + }(1000); r != 2350 { ppanic("r != 2350") } } { a := 2 - if r := func(x int) int { // ERROR "can inline main.func28" + if r := func(x int) int { // ERROR "func literal does not escape" b := 3 - return func(y int) int { // ERROR "can inline main.func28.1" "can inline main.func35" + return func(y int) int { // ERROR "can inline main.func28.1" c := 5 - func(z int) { // ERROR "can inline main.func28.1.1" "can inline main.func28.(func)?2" "can inline main.func35.1" "can inline main.func37" + func(z int) { // ERROR "can inline main.func28.1.1" "can inline main.func28.(func)?2" a = a * x b = b * y c = c * z }(10) // ERROR "inlining call to main.func28.1.1" return a + c }(100) + b // ERROR "inlining call to main.func28.1" "inlining call to main.func28.(func)?2" - }(1000); r != 2350 { // ERROR "inlining call to main.func28" "inlining call to main.func35" "inlining call to main.func37" + }(1000); r != 2350 { ppanic("r != 2350") } if a != 2000 { -- 2.48.1