]> Cypherpunks.ru repositories - gostls13.git/commitdiff
cmd/compile: add pgohash for debugging/bisecting PGO optimizations
authorCherry Mui <cherryyz@google.com>
Fri, 15 Sep 2023 16:13:09 +0000 (12:13 -0400)
committerCherry Mui <cherryyz@google.com>
Tue, 19 Sep 2023 15:42:58 +0000 (15:42 +0000)
When a PGO build fails or produces incorrect program, it is often
unclear what the problem is. Add pgo hash so we can bisect to
individual optimization decisions, which often helps debugging.

Related to #58153.

Change-Id: I651ffd9c53bad60f2f28c8ec2a90a3f532982712
Reviewed-on: https://go-review.googlesource.com/c/go/+/528400
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
src/cmd/compile/internal/base/debug.go
src/cmd/compile/internal/base/flag.go
src/cmd/compile/internal/base/hashdebug.go
src/cmd/compile/internal/devirtualize/pgo.go
src/cmd/compile/internal/inline/inl.go
src/cmd/compile/internal/test/pgo_inl_test.go

index 244ba1bc9d3e159c32daaf657f31de8be83cddf6..390ddf3176a12319648edd8f93658b54cbd9a15e 100644 (file)
@@ -55,6 +55,7 @@ type DebugFlags struct {
        ABIWrap               int    `help:"print information about ABI wrapper generation"`
        MayMoreStack          string `help:"call named function before all stack growth checks" concurrent:"ok"`
        PGODebug              int    `help:"debug profile-guided optimizations"`
+       PGOHash               string `help:"hash value for debugging profile-guided optimizations" concurrent:"ok"`
        PGOInline             int    `help:"enable profile-guided inlining" concurrent:"ok"`
        PGOInlineCDFThreshold string `help:"cumulative threshold percentage for determining call sites as hot candidates for inlining" concurrent:"ok"`
        PGOInlineBudget       int    `help:"inline budget for hot functions" concurrent:"ok"`
index 0e44deae71f51e22ae4d9206b2a522350d80712b..36340cb70bdd91d9ac651e2c0cd7631172cb67e8 100644 (file)
@@ -255,6 +255,9 @@ func ParseFlags() {
        if Debug.Fmahash != "" {
                FmaHash = NewHashDebug("fmahash", Debug.Fmahash, nil)
        }
+       if Debug.PGOHash != "" {
+               PGOHash = NewHashDebug("pgohash", Debug.PGOHash, nil)
+       }
 
        if Flag.MSan && !platform.MSanSupported(buildcfg.GOOS, buildcfg.GOARCH) {
                log.Fatalf("%s/%s does not support -msan", buildcfg.GOOS, buildcfg.GOARCH)
index 167b0df4f06ea65678d75db64b049ef2594a7cb0..de7f01f09e5b04be328656dd6e2b00c73db66fbf 100644 (file)
@@ -55,6 +55,7 @@ var hashDebug *HashDebug
 
 var FmaHash *HashDebug     // for debugging fused-multiply-add floating point changes
 var LoopVarHash *HashDebug // for debugging shared/private loop variable changes
+var PGOHash *HashDebug     // for debugging PGO optimization decisions
 
 // DebugHashMatchPkgFunc reports whether debug variable Gossahash
 //
@@ -274,8 +275,36 @@ func (d *HashDebug) MatchPos(pos src.XPos, desc func() string) bool {
 }
 
 func (d *HashDebug) matchPos(ctxt *obj.Link, pos src.XPos, note func() string) bool {
+       return d.matchPosWithInfo(ctxt, pos, nil, note)
+}
+
+func (d *HashDebug) matchPosWithInfo(ctxt *obj.Link, pos src.XPos, info any, note func() string) bool {
        hash := d.hashPos(ctxt, pos)
-       return d.matchAndLog(hash, func() string { return d.fmtPos(ctxt, pos) }, note)
+       if info != nil {
+               hash = bisect.Hash(hash, info)
+       }
+       return d.matchAndLog(hash,
+               func() string {
+                       r := d.fmtPos(ctxt, pos)
+                       if info != nil {
+                               r += fmt.Sprintf(" (%v)", info)
+                       }
+                       return r
+               },
+               note)
+}
+
+// MatchPosWithInfo is similar to MatchPos, but with additional information
+// that is included for hash computation, so it can distinguish multiple
+// matches on the same source location.
+// Note that the default answer for no environment variable (d == nil)
+// is "yes", do the thing.
+func (d *HashDebug) MatchPosWithInfo(pos src.XPos, info any, desc func() string) bool {
+       if d == nil {
+               return true
+       }
+       // Written this way to make inlining likely.
+       return d.matchPosWithInfo(Ctxt, pos, info, desc)
 }
 
 // matchAndLog is the core matcher. It reports whether the hash matches the pattern.
index b51028701ee214212f35047c0dcff7fc87ced5bf..a04ff16d6072910b7b8a79139442b0fbe8aea757 100644 (file)
@@ -155,6 +155,11 @@ func ProfileGuided(fn *ir.Func, p *pgo.Profile) {
                        return n
                }
 
+               if !base.PGOHash.MatchPosWithInfo(n.Pos(), "devirt", nil) {
+                       // De-selected by PGO Hash.
+                       return n
+               }
+
                if stat != nil {
                        stat.Devirtualized = ir.LinkFuncName(callee)
                        stat.DevirtualizedWeight = weight
index f1dce85afb295ce9eac112e5dcae25cbd51783e0..cd5adc14217de9cb056e7cc9fcf7dfc696d91122 100644 (file)
@@ -1044,6 +1044,11 @@ func inlineCostOK(n *ir.CallExpr, caller, callee *ir.Func, bigCaller bool) (bool
                return false, inlineHotMaxBudget
        }
 
+       if !base.PGOHash.MatchPosWithInfo(n.Pos(), "inline", nil) {
+               // De-selected by PGO Hash.
+               return false, maxCost
+       }
+
        if base.Debug.PGODebug > 0 {
                fmt.Printf("hot-budget check allows inlining for call %s (cost %d) at %v in function %s\n", ir.PkgFuncName(callee), callee.Inl.Cost, ir.Line(n), ir.PkgFuncName(caller))
        }
index 4d6b5a134a0e2808ae25da3f2f06bece30b50a19..7aabf8b01061420c4a0b70ea0dcf5896e5223f73 100644 (file)
@@ -6,6 +6,7 @@ package test
 
 import (
        "bufio"
+       "bytes"
        "fmt"
        "internal/profile"
        "internal/testenv"
@@ -17,11 +18,7 @@ import (
        "testing"
 )
 
-// testPGOIntendedInlining tests that specific functions are inlined.
-func testPGOIntendedInlining(t *testing.T, dir string) {
-       testenv.MustHaveGoRun(t)
-       t.Parallel()
-
+func buildPGOInliningTest(t *testing.T, dir string, flags ...string) []byte {
        const pkg = "example.com/pgo/inline"
 
        // Add a go.mod so we have a consistent symbol names in this temp dir.
@@ -32,6 +29,25 @@ go 1.19
                t.Fatalf("error writing go.mod: %v", err)
        }
 
+       exe := filepath.Join(dir, "test.exe")
+       args := []string{"test", "-c", "-o", exe}
+       args = append(args, flags...)
+       cmd := testenv.CleanCmdEnv(testenv.Command(t, testenv.GoToolPath(t), args...))
+       cmd.Dir = dir
+       out, err := cmd.CombinedOutput()
+       if err != nil {
+               t.Fatalf("build failed: %v, output:\n%s", err, out)
+       }
+       return out
+}
+
+// testPGOIntendedInlining tests that specific functions are inlined.
+func testPGOIntendedInlining(t *testing.T, dir string) {
+       testenv.MustHaveGoRun(t)
+       t.Parallel()
+
+       const pkg = "example.com/pgo/inline"
+
        want := []string{
                "(*BS).NS",
        }
@@ -71,25 +87,9 @@ go 1.19
        // TODO: maybe adjust the test to work with default threshold.
        pprof := filepath.Join(dir, "inline_hot.pprof")
        gcflag := fmt.Sprintf("-gcflags=-m -m -pgoprofile=%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90", pprof)
-       out := filepath.Join(dir, "test.exe")
-       cmd := testenv.CleanCmdEnv(testenv.Command(t, testenv.GoToolPath(t), "test", "-c", "-o", out, gcflag, "."))
-       cmd.Dir = dir
-
-       pr, pw, err := os.Pipe()
-       if err != nil {
-               t.Fatalf("error creating pipe: %v", err)
-       }
-       defer pr.Close()
-       cmd.Stdout = pw
-       cmd.Stderr = pw
-
-       err = cmd.Start()
-       pw.Close()
-       if err != nil {
-               t.Fatalf("error starting go test: %v", err)
-       }
+       out := buildPGOInliningTest(t, dir, gcflag)
 
-       scanner := bufio.NewScanner(pr)
+       scanner := bufio.NewScanner(bytes.NewReader(out))
        curPkg := ""
        canInline := regexp.MustCompile(`: can inline ([^ ]*)`)
        haveInlined := regexp.MustCompile(`: inlining call to ([^ ]*)`)
@@ -128,11 +128,8 @@ go 1.19
                        continue
                }
        }
-       if err := cmd.Wait(); err != nil {
-               t.Fatalf("error running go test: %v", err)
-       }
        if err := scanner.Err(); err != nil {
-               t.Fatalf("error reading go test output: %v", err)
+               t.Fatalf("error reading output: %v", err)
        }
        for fullName, reason := range notInlinedReason {
                t.Errorf("%s was not inlined: %s", fullName, reason)
@@ -297,3 +294,48 @@ func copyFile(dst, src string) error {
        _, err = io.Copy(d, s)
        return err
 }
+
+// TestPGOHash tests that PGO optimization decisions can be selected by pgohash.
+func TestPGOHash(t *testing.T) {
+       testenv.MustHaveGoRun(t)
+       t.Parallel()
+
+       wd, err := os.Getwd()
+       if err != nil {
+               t.Fatalf("error getting wd: %v", err)
+       }
+       srcDir := filepath.Join(wd, "testdata/pgo/inline")
+
+       // 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"} {
+               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")
+       gcflag0 := fmt.Sprintf("-gcflags=-pgoprofile=%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90,pgodebug=1,", pprof)
+
+       // Check that a hash match allows PGO inlining.
+       const srcPos = "example.com/pgo/inline/inline_hot.go:81:19"
+       const hashMatch = "pgohash triggered " + srcPos + " (inline)"
+       pgoDebugRE := regexp.MustCompile(`hot-budget check allows inlining for call .* at ` + strings.ReplaceAll(srcPos, ".", "\\."))
+       hash := "v1" // 1 matches srcPos, v for verbose (print source location)
+       gcflag := gcflag0 + ",pgohash=" + hash
+       // build with -trimpath so the source location (thus the hash)
+       // does not depend on the temporary directory path.
+       out := buildPGOInliningTest(t, dir, gcflag, "-trimpath")
+       if !bytes.Contains(out, []byte(hashMatch)) || !pgoDebugRE.Match(out) {
+               t.Errorf("output does not contain expected source line, out:\n%s", out)
+       }
+
+       // Check that a hash mismatch turns off PGO inlining.
+       hash = "v0" // 0 should not match srcPos
+       gcflag = gcflag0 + ",pgohash=" + hash
+       out = buildPGOInliningTest(t, dir, gcflag, "-trimpath")
+       if bytes.Contains(out, []byte(hashMatch)) || pgoDebugRE.Match(out) {
+               t.Errorf("output contains unexpected source line, out:\n%s", out)
+       }
+}