cmd/compile: update the incorrect assignment of call site offset.

The call site calculation in the previous version is incorrect. For
the PGO preprocess file, the compiler should directly use the call
site offset value. Additionly, this change refactors the preprocess
tool to clean up unused fields including startline, the flat and the
cum.

Change-Id: I7bffed3215d4c016d9a9e4034bfd373bf50ab43f
Reviewed-on: https://go-review.googlesource.com/c/go/+/562795
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>

diff --git a/src/cmd/compile/internal/pgo/irgraph.go b/src/cmd/compile/internal/pgo/irgraph.go
index 224f14368fd70507e794f90617c14b214514cc8a..9ed16d224b85051e210fe4b8e3eb35cb054e853c 100644 (file)
--- a/src/cmd/compile/internal/pgo/irgraph.go
+++ b/src/cmd/compile/internal/pgo/irgraph.go
@@ -108,7 +108,6 @@ type NamedCallEdge struct {
        CallerName     string
        CalleeName     string
        CallSiteOffset int // Line offset from function start line.
-       CallStartLine  int // Start line of the function. Can be 0 which means missing.
 }
 
 // NamedEdgeMap contains all unique call edges in the profile and their
@@ -336,20 +335,19 @@ func createNamedEdgeMapFromPreprocess(r io.Reader) (edgeMap NamedEdgeMap, totalW
 
                split := strings.Split(readStr, " ")
 
-               if len(split) != 5 {
-                       return NamedEdgeMap{}, 0, fmt.Errorf("preprocessed profile entry got %v want 5 fields", split)
+               if len(split) != 2 {
+                       return NamedEdgeMap{}, 0, fmt.Errorf("preprocessed profile entry got %v want 2 fields", split)
                }
 
                co, _ := strconv.Atoi(split[0])
-               cs, _ := strconv.Atoi(split[1])
 
                namedEdge := NamedCallEdge{
                        CallerName:     callerName,
-                       CallSiteOffset: co - cs,
+                       CalleeName:     calleeName,
+                       CallSiteOffset: co,
                }
 
-               namedEdge.CalleeName = calleeName
-               EWeight, _ := strconv.ParseInt(split[4], 10, 64)
+               EWeight, _ := strconv.ParseInt(split[1], 10, 64)
 
                weight[namedEdge] += EWeight
                totalWeight += EWeight

diff --git a/src/cmd/compile/internal/test/pgo_devirtualize_test.go b/src/cmd/compile/internal/test/pgo_devirtualize_test.go
index f4512436834fc56ef5de13ad97a297942534a0b0..af09107dc04d5acfe267ad29c694643ddc274c51 100644 (file)
--- a/src/cmd/compile/internal/test/pgo_devirtualize_test.go
+++ b/src/cmd/compile/internal/test/pgo_devirtualize_test.go
@@ -19,8 +19,11 @@ type devirtualization struct {
        callee string
 }
 
+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) {
+func testPGODevirtualize(t *testing.T, dir string, want []devirtualization, pgoProfileName string) {
        testenv.MustHaveGoRun(t)
        t.Parallel()
 
@@ -45,7 +48,7 @@ go 1.21
        }
 
        // Build the test with the profile.
-       pprof := filepath.Join(dir, "devirt.pprof")
+       pprof := filepath.Join(dir, pgoProfileName)
        gcflag := fmt.Sprintf("-gcflags=-m=2 -pgoprofile=%s -d=pgodebug=3", pprof)
        out := filepath.Join(dir, "test.exe")
        cmd = testenv.CleanCmdEnv(testenv.Command(t, testenv.GoToolPath(t), "test", "-o", out, gcflag, "."))
@@ -126,7 +129,70 @@ func TestPGODevirtualize(t *testing.T) {
        if err := os.Mkdir(filepath.Join(dir, "mult.pkg"), 0755); err != nil {
                t.Fatalf("error creating dir: %v", err)
        }
-       for _, file := range []string{"devirt.go", "devirt_test.go", "devirt.pprof", filepath.Join("mult.pkg", "mult.go")} {
+       for _, file := range []string{"devirt.go", "devirt_test.go", profFileName, filepath.Join("mult.pkg", "mult.go")} {
+               if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil {
+                       t.Fatalf("error copying %s: %v", file, err)
+               }
+       }
+
+       want := []devirtualization{
+               // ExerciseIface
+               {
+                       pos:    "./devirt.go:101:20",
+                       callee: "mult.Mult.Multiply",
+               },
+               {
+                       pos:    "./devirt.go:101:39",
+                       callee: "Add.Add",
+               },
+               // ExerciseFuncConcrete
+               {
+                       pos:    "./devirt.go:173:36",
+                       callee: "AddFn",
+               },
+               {
+                       pos:    "./devirt.go:173:15",
+                       callee: "mult.MultFn",
+               },
+               // ExerciseFuncField
+               {
+                       pos:    "./devirt.go:207:35",
+                       callee: "AddFn",
+               },
+               {
+                       pos:    "./devirt.go:207:19",
+                       callee: "mult.MultFn",
+               },
+               // ExerciseFuncClosure
+               // TODO(prattmic): Closure callees not implemented.
+               //{
+               //      pos:    "./devirt.go:249:27",
+               //      callee: "AddClosure.func1",
+               //},
+               //{
+               //      pos:    "./devirt.go:249:15",
+               //      callee: "mult.MultClosure.func1",
+               //},
+       }
+
+       testPGODevirtualize(t, dir, want, profFileName)
+}
+
+// TestPGOPreprocessDevirtualize tests that specific functions are devirtualized when PGO
+// is applied to the exact source that was profiled. The input profile is PGO preprocessed file.
+func TestPGOPreprocessDevirtualize(t *testing.T) {
+       wd, err := os.Getwd()
+       if err != nil {
+               t.Fatalf("error getting wd: %v", err)
+       }
+       srcDir := filepath.Join(wd, "testdata", "pgo", "devirtualize")
+
+       // Copy the module to a scratch location so we can add a go.mod.
+       dir := t.TempDir()
+       if err := os.Mkdir(filepath.Join(dir, "mult.pkg"), 0755); err != nil {
+               t.Fatalf("error creating dir: %v", err)
+       }
+       for _, file := range []string{"devirt.go", "devirt_test.go", preProfFileName, filepath.Join("mult.pkg", "mult.go")} {
                if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil {
                        t.Fatalf("error copying %s: %v", file, err)
                }
@@ -172,7 +238,7 @@ func TestPGODevirtualize(t *testing.T) {
                //},
        }
 
-       testPGODevirtualize(t, dir, want)
+       testPGODevirtualize(t, dir, want, preProfFileName)
 }
 
 // Regression test for https://go.dev/issue/65615. If a target function changes
@@ -190,7 +256,7 @@ func TestLookupFuncGeneric(t *testing.T) {
        if err := os.Mkdir(filepath.Join(dir, "mult.pkg"), 0755); err != nil {
                t.Fatalf("error creating dir: %v", err)
        }
-       for _, file := range []string{"devirt.go", "devirt_test.go", "devirt.pprof", filepath.Join("mult.pkg", "mult.go")} {
+       for _, file := range []string{"devirt.go", "devirt_test.go", profFileName, filepath.Join("mult.pkg", "mult.go")} {
                if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil {
                        t.Fatalf("error copying %s: %v", file, err)
                }
@@ -238,7 +304,7 @@ func TestLookupFuncGeneric(t *testing.T) {
                //},
        }
 
-       testPGODevirtualize(t, dir, want)
+       testPGODevirtualize(t, dir, want, profFileName)
 }
 
 var multFnRe = regexp.MustCompile(`func MultFn\(a, b int64\) int64`)

diff --git a/src/cmd/compile/internal/test/pgo_inl_test.go b/src/cmd/compile/internal/test/pgo_inl_test.go
index 3aafaee197965eac36b6f9335e2e2d3d6af9f959..7d665655d5746115e84d4b486e404e7b820a7625 100644 (file)
--- a/src/cmd/compile/internal/test/pgo_inl_test.go
+++ b/src/cmd/compile/internal/test/pgo_inl_test.go
@@ -18,6 +18,9 @@ import (
        "testing"
 )
 
+const profFile = "inline_hot.pprof"
+const preProfFile = "inline_hot.pprof.node_map"
+
 func buildPGOInliningTest(t *testing.T, dir string, gcflag string) []byte {
        const pkg = "example.com/pgo/inline"
 
@@ -43,12 +46,7 @@ go 1.19
 }
 
 // testPGOIntendedInlining tests that specific functions are inlined.
-func testPGOIntendedInlining(t *testing.T, dir string, preprocessed ...bool) {
-       defaultPGOPackValue := false
-       if len(preprocessed) > 0 {
-               defaultPGOPackValue = preprocessed[0]
-       }
-
+func testPGOIntendedInlining(t *testing.T, dir string, profFile string) {
        testenv.MustHaveGoRun(t)
        t.Parallel()
 
@@ -91,13 +89,7 @@ func testPGOIntendedInlining(t *testing.T, dir string, preprocessed ...bool) {
 
        // Build the test with the profile. Use a smaller threshold to test.
        // TODO: maybe adjust the test to work with default threshold.
-       var pprof string
-       if defaultPGOPackValue == false {
-               pprof = filepath.Join(dir, "inline_hot.pprof")
-       } else {
-               pprof = filepath.Join(dir, "inline_hot.pprof.node_map")
-       }
-       gcflag := fmt.Sprintf("-m -m -pgoprofile=%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90", pprof)
+       gcflag := fmt.Sprintf("-m -m -pgoprofile=%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90", profFile)
        out := buildPGOInliningTest(t, dir, gcflag)
 
        scanner := bufio.NewScanner(bytes.NewReader(out))
@@ -165,13 +157,13 @@ func TestPGOIntendedInlining(t *testing.T) {
        // Copy the module to a scratch location so we can add a go.mod.
        dir := t.TempDir()
 
-       for _, file := range []string{"inline_hot.go", "inline_hot_test.go", "inline_hot.pprof"} {
+       for _, file := range []string{"inline_hot.go", "inline_hot_test.go", profFile} {
                if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil {
                        t.Fatalf("error copying %s: %v", file, err)
                }
        }
 
-       testPGOIntendedInlining(t, dir)
+       testPGOIntendedInlining(t, dir, profFile)
 }
 
 // TestPGOIntendedInlining tests that specific functions are inlined when PGO
@@ -186,13 +178,13 @@ func TestPGOPreprocessInlining(t *testing.T) {
        // Copy the module to a scratch location so we can add a go.mod.
        dir := t.TempDir()
 
-       for _, file := range []string{"inline_hot.go", "inline_hot_test.go", "inline_hot.pprof.node_map"} {
+       for _, file := range []string{"inline_hot.go", "inline_hot_test.go", preProfFile} {
                if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil {
                        t.Fatalf("error copying %s: %v", file, err)
                }
        }
 
-       testPGOIntendedInlining(t, dir, true)
+       testPGOIntendedInlining(t, dir, preProfFile)
 }
 
 // TestPGOIntendedInlining tests that specific functions are inlined when PGO
@@ -208,7 +200,7 @@ func TestPGOIntendedInliningShiftedLines(t *testing.T) {
        dir := t.TempDir()
 
        // Copy most of the files unmodified.
-       for _, file := range []string{"inline_hot_test.go", "inline_hot.pprof"} {
+       for _, file := range []string{"inline_hot_test.go", profFile} {
                if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil {
                        t.Fatalf("error copying %s : %v", file, err)
                }
@@ -240,7 +232,7 @@ func TestPGOIntendedInliningShiftedLines(t *testing.T) {
 
        dst.Close()
 
-       testPGOIntendedInlining(t, dir)
+       testPGOIntendedInlining(t, dir, profFile)
 }
 
 // TestPGOSingleIndex tests that the sample index can not be 1 and compilation
@@ -270,15 +262,15 @@ func TestPGOSingleIndex(t *testing.T) {
                        // Copy the module to a scratch location so we can add a go.mod.
                        dir := t.TempDir()
 
-                       originalPprofFile, err := os.Open(filepath.Join(srcDir, "inline_hot.pprof"))
+                       originalPprofFile, err := os.Open(filepath.Join(srcDir, profFile))
                        if err != nil {
-                               t.Fatalf("error opening inline_hot.pprof: %v", err)
+                               t.Fatalf("error opening %v: %v", profFile, err)
                        }
                        defer originalPprofFile.Close()
 
                        p, err := profile.Parse(originalPprofFile)
                        if err != nil {
-                               t.Fatalf("error parsing inline_hot.pprof: %v", err)
+                               t.Fatalf("error parsing %v: %v", profFile, err)
                        }
 
                        // Move the samples count value-type to the 0 index.
@@ -289,14 +281,14 @@ func TestPGOSingleIndex(t *testing.T) {
                                s.Value = []int64{s.Value[tc.originalIndex]}
                        }
 
-                       modifiedPprofFile, err := os.Create(filepath.Join(dir, "inline_hot.pprof"))
+                       modifiedPprofFile, err := os.Create(filepath.Join(dir, profFile))
                        if err != nil {
-                               t.Fatalf("error creating inline_hot.pprof: %v", err)
+                               t.Fatalf("error creating %v: %v", profFile, err)
                        }
                        defer modifiedPprofFile.Close()
 
                        if err := p.Write(modifiedPprofFile); err != nil {
-                               t.Fatalf("error writing inline_hot.pprof: %v", err)
+                               t.Fatalf("error writing %v: %v", profFile, err)
                        }
 
                        for _, file := range []string{"inline_hot.go", "inline_hot_test.go"} {
@@ -305,7 +297,7 @@ func TestPGOSingleIndex(t *testing.T) {
                                }
                        }
 
-                       testPGOIntendedInlining(t, dir)
+                       testPGOIntendedInlining(t, dir, profFile)
                })
        }
 }
@@ -343,13 +335,13 @@ func TestPGOHash(t *testing.T) {
        // Copy the module to a scratch location so we can add a go.mod.
        dir := t.TempDir()
 
-       for _, file := range []string{"inline_hot.go", "inline_hot_test.go", "inline_hot.pprof"} {
+       for _, file := range []string{"inline_hot.go", "inline_hot_test.go", profFile} {
                if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil {
                        t.Fatalf("error copying %s: %v", file, err)
                }
        }
 
-       pprof := filepath.Join(dir, "inline_hot.pprof")
+       pprof := filepath.Join(dir, profFile)
        // build with -trimpath so the source location (thus the hash)
        // does not depend on the temporary directory path.
        gcflag0 := fmt.Sprintf("-pgoprofile=%s -trimpath %s=>%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90,pgodebug=1", pprof, dir, pkg)

diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.pprof.node_map b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.pprof.node_map
new file mode 100644 (file)
index 0000000..c55f990
--- /dev/null
+++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.pprof.node_map
@@ -0,0 +1,52 @@
+GO PREPROFILE V1
+example.com/pgo/devirtualize.ExerciseFuncClosure
+example.com/pgo/devirtualize/mult%2epkg.MultClosure.func1
+18 93
+example.com/pgo/devirtualize.ExerciseIface
+example.com/pgo/devirtualize/mult%2epkg.NegMult.Multiply
+49 4
+example.com/pgo/devirtualize.ExerciseFuncConcrete
+example.com/pgo/devirtualize.AddFn
+48 103
+example.com/pgo/devirtualize.ExerciseFuncField
+example.com/pgo/devirtualize/mult%2epkg.NegMultFn
+23 8
+example.com/pgo/devirtualize.ExerciseFuncField
+example.com/pgo/devirtualize/mult%2epkg.MultFn
+23 94
+example.com/pgo/devirtualize.ExerciseIface
+example.com/pgo/devirtualize/mult%2epkg.Mult.Multiply
+49 40
+example.com/pgo/devirtualize.ExerciseIface
+example.com/pgo/devirtualize.Add.Add
+49 55
+example.com/pgo/devirtualize.ExerciseFuncConcrete
+example.com/pgo/devirtualize/mult%2epkg.NegMultFn
+48 8
+example.com/pgo/devirtualize.ExerciseFuncClosure
+example.com/pgo/devirtualize/mult%2epkg.NegMultClosure.func1
+18 10
+example.com/pgo/devirtualize.ExerciseIface
+example.com/pgo/devirtualize.Sub.Add
+49 7
+example.com/pgo/devirtualize.ExerciseFuncField
+example.com/pgo/devirtualize.AddFn
+23 101
+example.com/pgo/devirtualize.ExerciseFuncField
+example.com/pgo/devirtualize.SubFn
+23 12
+example.com/pgo/devirtualize.BenchmarkDevirtFuncConcrete
+example.com/pgo/devirtualize.ExerciseFuncConcrete
+1 2
+example.com/pgo/devirtualize.ExerciseFuncConcrete
+example.com/pgo/devirtualize/mult%2epkg.MultFn
+48 91
+example.com/pgo/devirtualize.ExerciseFuncConcrete
+example.com/pgo/devirtualize.SubFn
+48 5
+example.com/pgo/devirtualize.ExerciseFuncClosure
+example.com/pgo/devirtualize.Add.Add
+18 92
+example.com/pgo/devirtualize.ExerciseFuncClosure
+example.com/pgo/devirtualize.Sub.Add
+18 14

diff --git a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof.node_map b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof.node_map
index bc5bc66b61436443c453af89b7206fa1c4eefb59..6e5f937a505bc4cefdd33ddb88d357f8af45c8ef 100644 (file)
--- a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof.node_map
+++ b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof.node_map
@@ -1,13 +1,13 @@
 GO PREPROFILE V1
 example.com/pgo/inline.benchmarkB
 example.com/pgo/inline.A
-18 17 0 1 1
+18 1
 example.com/pgo/inline.(*BS).NS
 example.com/pgo/inline.T
-13 53 124 129 2
+8 3
 example.com/pgo/inline.(*BS).NS
 example.com/pgo/inline.T
-8 53 124 129 3
+13 2
 example.com/pgo/inline.A
 example.com/pgo/inline.(*BS).NS
-7 74 1 130 129
+7 129

diff --git a/src/cmd/preprofile/main.go b/src/cmd/preprofile/main.go
index cf747266ca8be30ee33f4b728de444df761ff803..806f25fee8b6c6592a65221ca30ebb7dc62e50a1 100644 (file)
--- a/src/cmd/preprofile/main.go
+++ b/src/cmd/preprofile/main.go
@@ -35,11 +35,11 @@ import (
 //     Header
 //     caller_name
 //      callee_name
-//      "call site offset" "caller's start line number" "flat" "cum" "call edge weight"
+//      "call site offset" "call edge weight"
 //      ...
 //     caller_name
 //      callee_name
-//      "call site offset" "caller's start line number" "flat" "cum" "call edge weight"
+//      "call site offset" "call edge weight"
 
 func usage() {
        fmt.Fprintf(os.Stderr, "MUST have (pprof) input file \n")
@@ -52,13 +52,6 @@ type NodeMapKey struct {
        CallerName     string
        CalleeName     string
        CallSiteOffset int // Line offset from function start line.
-       CallStartLine  int // Start line of the function. Can be 0 which means missing.
-}
-
-type Weights struct {
-       NFlat   int64
-       NCum    int64
-       EWeight int64
 }
 
 func readPprofFile(profileFile string, outputFile string, verbose bool) bool {
@@ -101,33 +94,19 @@ func readPprofFile(profileFile string, outputFile string, verbose bool) bool {
                SampleValue: func(v []int64) int64 { return v[valueIndex] },
        })
 
-       nFlat := make(map[string]int64)
-       nCum := make(map[string]int64)
-
-       // Accummulate weights for the same node.
-       for _, n := range g.Nodes {
-               canonicalName := n.Info.Name
-               nFlat[canonicalName] += n.FlatValue()
-               nCum[canonicalName] += n.CumValue()
-       }
-
-       TotalNodeWeight := int64(0)
        TotalEdgeWeight := int64(0)
 
-       NodeMap := make(map[NodeMapKey]*Weights)
-       NodeWeightMap := make(map[string]int64)
+       NodeMap := make(map[NodeMapKey]int64)
 
        for _, n := range g.Nodes {
-               TotalNodeWeight += n.FlatValue()
                canonicalName := n.Info.Name
                // Create the key to the nodeMapKey.
                nodeinfo := NodeMapKey{
                        CallerName:     canonicalName,
                        CallSiteOffset: n.Info.Lineno - n.Info.StartLine,
-                       CallStartLine:  n.Info.StartLine,
                }
 
-               if nodeinfo.CallStartLine == 0 {
+               if n.Info.StartLine == 0 {
                        if verbose {
                                log.Println("[PGO] warning: " + canonicalName + " relative line number is missing from the profile")
                        }
@@ -137,27 +116,14 @@ func readPprofFile(profileFile string, outputFile string, verbose bool) bool {
                        TotalEdgeWeight += e.WeightValue()
                        nodeinfo.CalleeName = e.Dest.Info.Name
                        if w, ok := NodeMap[nodeinfo]; ok {
-                               w.EWeight += e.WeightValue()
+                               w += e.WeightValue()
                        } else {
-                               weights := new(Weights)
-                               weights.NFlat = nFlat[canonicalName]
-                               weights.NCum = nCum[canonicalName]
-                               weights.EWeight = e.WeightValue()
-                               NodeMap[nodeinfo] = weights
+                               w = e.WeightValue()
+                               NodeMap[nodeinfo] = w
                        }
                }
        }
 
-       for _, n := range g.Nodes {
-               lineno := fmt.Sprintf("%v", n.Info.Lineno)
-               canonicalName := n.Info.Name + "-" + lineno
-               if _, ok := (NodeWeightMap)[canonicalName]; ok {
-                       (NodeWeightMap)[canonicalName] += n.CumValue()
-               } else {
-                       (NodeWeightMap)[canonicalName] = n.CumValue()
-               }
-       }
-
        var fNodeMap *os.File
        if outputFile == "" {
                fNodeMap = os.Stdout
@@ -190,16 +156,13 @@ func readPprofFile(profileFile string, outputFile string, verbose bool) bool {
                line = key.CalleeName + "\n"
                w.WriteString(line)
                line = strconv.Itoa(key.CallSiteOffset)
-               line = line + separator + strconv.Itoa(key.CallStartLine)
-               line = line + separator + strconv.FormatInt(element.NFlat, 10)
-               line = line + separator + strconv.FormatInt(element.NCum, 10)
-               line = line + separator + strconv.FormatInt(element.EWeight, 10) + "\n"
+               line = line + separator + strconv.FormatInt(element, 10) + "\n"
                w.WriteString(line)
                w.Flush()
                count += 1
        }
 
-       if TotalNodeWeight == 0 || TotalEdgeWeight == 0 {
+       if TotalEdgeWeight == 0 {
                return false
        }