cmd/compile/internal/devirtualize: do not select a zero-weight edge as the hottest one

When both a direct call and an interface call appear on the same line,
PGO devirtualization may make a suboptimal decision. In some cases,
the directly called function becomes a candidate for devirtualization
if no other relevant outgoing edges with non-zero weight exist for the
caller's IRNode in the WeightedCG. The edge to this candidate is
considered the hottest. Despite having zero weight, this edge still
causes the interface call to be devirtualized.

This CL prevents devirtualization when the weight of the hottest edge
is 0.

Fixes #72092

Change-Id: I06c0c5e080398d86f832e09244aceaa4aeb98721
Reviewed-on: https://go-review.googlesource.com/c/go/+/655475
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/cmd/compile/internal/devirtualize/pgo.go b/src/cmd/compile/internal/devirtualize/pgo.go
index 96c9231be3f13a8d3772930a8971d9aa9732e344..a8980bb86b1547447b85986b9d1132359e042928 100644 (file)
--- a/src/cmd/compile/internal/devirtualize/pgo.go
+++ b/src/cmd/compile/internal/devirtualize/pgo.go
@@ -741,7 +741,7 @@ func findHotConcreteCallee(p *pgoir.Profile, caller *ir.Func, call *ir.CallExpr,
                hottest = e
        }
 
-       if hottest == nil {
+       if hottest == nil || hottest.Weight == 0 {
                if base.Debug.PGODebug >= 2 {
                        fmt.Printf("%v: call %s:%d: no hot callee\n", ir.Line(call), callerName, callOffset)
                }

diff --git a/src/cmd/compile/internal/test/pgo_devirtualize_test.go b/src/cmd/compile/internal/test/pgo_devirtualize_test.go
index af09107dc04d5acfe267ad29c694643ddc274c51..a97a5799fc72c5ad1f125bba1eb0d83ff9bc9ff4 100644 (file)
--- a/src/cmd/compile/internal/test/pgo_devirtualize_test.go
+++ b/src/cmd/compile/internal/test/pgo_devirtualize_test.go
@@ -23,7 +23,7 @@ const profFileName = "devirt.pprof"
 const preProfFileName = "devirt.pprof.node_map"
 
 // testPGODevirtualize tests that specific PGO devirtualize rewrites are performed.
-func testPGODevirtualize(t *testing.T, dir string, want []devirtualization, pgoProfileName string) {
+func testPGODevirtualize(t *testing.T, dir string, want, nowant []devirtualization, pgoProfileName string) {
        testenv.MustHaveGoRun(t)
        t.Parallel()
 
@@ -69,8 +69,10 @@ go 1.21
        }
 
        got := make(map[devirtualization]struct{})
+       gotNoHot := make(map[devirtualization]struct{})
 
        devirtualizedLine := regexp.MustCompile(`(.*): PGO devirtualizing \w+ call .* to (.*)`)
+       noHotLine := regexp.MustCompile(`(.*): call .*: no hot callee`)
 
        scanner := bufio.NewScanner(pr)
        for scanner.Scan() {
@@ -78,15 +80,21 @@ go 1.21
                t.Logf("child: %s", line)
 
                m := devirtualizedLine.FindStringSubmatch(line)
-               if m == nil {
+               if m != nil {
+                       d := devirtualization{
+                               pos:    m[1],
+                               callee: m[2],
+                       }
+                       got[d] = struct{}{}
                        continue
                }
-
-               d := devirtualization{
-                       pos:    m[1],
-                       callee: m[2],
+               m = noHotLine.FindStringSubmatch(line)
+               if m != nil {
+                       d := devirtualization{
+                               pos: m[1],
+                       }
+                       gotNoHot[d] = struct{}{}
                }
-               got[d] = struct{}{}
        }
        if err := cmd.Wait(); err != nil {
                t.Fatalf("error running go test: %v", err)
@@ -104,6 +112,11 @@ go 1.21
                }
                t.Errorf("devirtualization %v missing; got %v", w, got)
        }
+       for _, nw := range nowant {
+               if _, ok := gotNoHot[nw]; !ok {
+                       t.Errorf("unwanted devirtualization %v; got %v", nw, got)
+               }
+       }
 
        // Run test with PGO to ensure the assertions are still true.
        cmd = testenv.CleanCmdEnv(testenv.Command(t, out))
@@ -174,8 +187,18 @@ func TestPGODevirtualize(t *testing.T) {
                //      callee: "mult.MultClosure.func1",
                //},
        }
+       nowant := []devirtualization{
+               // ExerciseIfaceZeroWeight
+               {
+                       pos: "./devirt.go:256:29",
+               },
+               // ExerciseIndirCallZeroWeight
+               {
+                       pos: "./devirt.go:282:37",
+               },
+       }
 
-       testPGODevirtualize(t, dir, want, profFileName)
+       testPGODevirtualize(t, dir, want, nowant, profFileName)
 }
 
 // TestPGOPreprocessDevirtualize tests that specific functions are devirtualized when PGO
@@ -237,8 +260,18 @@ func TestPGOPreprocessDevirtualize(t *testing.T) {
                //      callee: "mult.MultClosure.func1",
                //},
        }
+       nowant := []devirtualization{
+               // ExerciseIfaceZeroWeight
+               {
+                       pos: "./devirt.go:256:29",
+               },
+               // ExerciseIndirCallZeroWeight
+               {
+                       pos: "./devirt.go:282:37",
+               },
+       }
 
-       testPGODevirtualize(t, dir, want, preProfFileName)
+       testPGODevirtualize(t, dir, want, nowant, preProfFileName)
 }
 
 // Regression test for https://go.dev/issue/65615. If a target function changes
@@ -303,8 +336,18 @@ func TestLookupFuncGeneric(t *testing.T) {
                //      callee: "mult.MultClosure.func1",
                //},
        }
+       nowant := []devirtualization{
+               // ExerciseIfaceZeroWeight
+               {
+                       pos: "./devirt.go:256:29",
+               },
+               // ExerciseIndirCallZeroWeight
+               {
+                       pos: "./devirt.go:282:37",
+               },
+       }
 
-       testPGODevirtualize(t, dir, want, profFileName)
+       testPGODevirtualize(t, dir, want, nowant, profFileName)
 }
 
 var multFnRe = regexp.MustCompile(`func MultFn\(a, b int64\) int64`)

diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go
index ac238f6dea42a90c375cc32c7d09c8d8e735024e..129809e3860d930b1c433a4e12ed2165b065d02e 100644 (file)
--- a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go
+++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go
@@ -250,3 +250,45 @@ func ExerciseFuncClosure(iter int, a1, a2 AddFunc, m1, m2 mult.MultFunc) int {
        }
        return val
 }
+
+//go:noinline
+func IfaceZeroWeight(a *Add, b Adder) bool {
+       return a.Add(1, 2) == b.Add(3, 4) // unwanted devirtualization
+}
+
+// ExerciseIfaceZeroWeight never calls IfaceZeroWeight, so the callee
+// is not expected to appear in the profile.
+//
+//go:noinline
+func ExerciseIfaceZeroWeight() {
+       if false {
+               a := &Add{}
+               b := &Sub{}
+               // Unreachable call
+               IfaceZeroWeight(a, b)
+       }
+}
+
+func DirectCall() bool {
+       return true
+}
+
+func IndirectCall() bool {
+       return false
+}
+
+//go:noinline
+func IndirCallZeroWeight(indirectCall func() bool) bool {
+       return DirectCall() && indirectCall() // unwanted devirtualization
+}
+
+// ExerciseIndirCallZeroWeight never calls IndirCallZeroWeight, so the
+// callee is not expected to appear in the profile.
+//
+//go:noinline
+func ExerciseIndirCallZeroWeight() {
+       if false {
+               // Unreachable call
+               IndirCallZeroWeight(IndirectCall)
+       }
+}

diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go
index 59b565d77fa7764a478a13cb4922751a52c9473a..2116e9b249f8c8542e0fa7979dbf2c71b28a92eb 100644 (file)
--- a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go
+++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go
@@ -71,3 +71,19 @@ func TestDevirtFuncClosure(t *testing.T) {
                t.Errorf("ExerciseFuncClosure(10) got %d want 1176", v)
        }
 }
+
+func BenchmarkDevirtIfaceZeroWeight(t *testing.B) {
+       ExerciseIfaceZeroWeight()
+}
+
+func TestDevirtIfaceZeroWeight(t *testing.T) {
+       ExerciseIfaceZeroWeight()
+}
+
+func BenchmarkDevirtIndirCallZeroWeight(t *testing.B) {
+       ExerciseIndirCallZeroWeight()
+}
+
+func TestDevirtIndirCallZeroWeight(t *testing.T) {
+       ExerciseIndirCallZeroWeight()
+}