Skip to content

Commit 3fb86fb

Browse files
committed
cmd/compile: add pgohash for debugging/bisecting PGO optimizations
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 <[email protected]> Reviewed-by: Michael Pratt <[email protected]>
1 parent eca5a97 commit 3fb86fb

File tree

6 files changed

+113
-28
lines changed

6 files changed

+113
-28
lines changed

src/cmd/compile/internal/base/debug.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ type DebugFlags struct {
5555
ABIWrap int `help:"print information about ABI wrapper generation"`
5656
MayMoreStack string `help:"call named function before all stack growth checks" concurrent:"ok"`
5757
PGODebug int `help:"debug profile-guided optimizations"`
58+
PGOHash string `help:"hash value for debugging profile-guided optimizations" concurrent:"ok"`
5859
PGOInline int `help:"enable profile-guided inlining" concurrent:"ok"`
5960
PGOInlineCDFThreshold string `help:"cumulative threshold percentage for determining call sites as hot candidates for inlining" concurrent:"ok"`
6061
PGOInlineBudget int `help:"inline budget for hot functions" concurrent:"ok"`

src/cmd/compile/internal/base/flag.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,9 @@ func ParseFlags() {
255255
if Debug.Fmahash != "" {
256256
FmaHash = NewHashDebug("fmahash", Debug.Fmahash, nil)
257257
}
258+
if Debug.PGOHash != "" {
259+
PGOHash = NewHashDebug("pgohash", Debug.PGOHash, nil)
260+
}
258261

259262
if Flag.MSan && !platform.MSanSupported(buildcfg.GOOS, buildcfg.GOARCH) {
260263
log.Fatalf("%s/%s does not support -msan", buildcfg.GOOS, buildcfg.GOARCH)

src/cmd/compile/internal/base/hashdebug.go

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ var hashDebug *HashDebug
5555

5656
var FmaHash *HashDebug // for debugging fused-multiply-add floating point changes
5757
var LoopVarHash *HashDebug // for debugging shared/private loop variable changes
58+
var PGOHash *HashDebug // for debugging PGO optimization decisions
5859

5960
// DebugHashMatchPkgFunc reports whether debug variable Gossahash
6061
//
@@ -274,8 +275,36 @@ func (d *HashDebug) MatchPos(pos src.XPos, desc func() string) bool {
274275
}
275276

276277
func (d *HashDebug) matchPos(ctxt *obj.Link, pos src.XPos, note func() string) bool {
278+
return d.matchPosWithInfo(ctxt, pos, nil, note)
279+
}
280+
281+
func (d *HashDebug) matchPosWithInfo(ctxt *obj.Link, pos src.XPos, info any, note func() string) bool {
277282
hash := d.hashPos(ctxt, pos)
278-
return d.matchAndLog(hash, func() string { return d.fmtPos(ctxt, pos) }, note)
283+
if info != nil {
284+
hash = bisect.Hash(hash, info)
285+
}
286+
return d.matchAndLog(hash,
287+
func() string {
288+
r := d.fmtPos(ctxt, pos)
289+
if info != nil {
290+
r += fmt.Sprintf(" (%v)", info)
291+
}
292+
return r
293+
},
294+
note)
295+
}
296+
297+
// MatchPosWithInfo is similar to MatchPos, but with additional information
298+
// that is included for hash computation, so it can distinguish multiple
299+
// matches on the same source location.
300+
// Note that the default answer for no environment variable (d == nil)
301+
// is "yes", do the thing.
302+
func (d *HashDebug) MatchPosWithInfo(pos src.XPos, info any, desc func() string) bool {
303+
if d == nil {
304+
return true
305+
}
306+
// Written this way to make inlining likely.
307+
return d.matchPosWithInfo(Ctxt, pos, info, desc)
279308
}
280309

281310
// matchAndLog is the core matcher. It reports whether the hash matches the pattern.

src/cmd/compile/internal/devirtualize/pgo.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,11 @@ func ProfileGuided(fn *ir.Func, p *pgo.Profile) {
155155
return n
156156
}
157157

158+
if !base.PGOHash.MatchPosWithInfo(n.Pos(), "devirt", nil) {
159+
// De-selected by PGO Hash.
160+
return n
161+
}
162+
158163
if stat != nil {
159164
stat.Devirtualized = ir.LinkFuncName(callee)
160165
stat.DevirtualizedWeight = weight

src/cmd/compile/internal/inline/inl.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,6 +1044,11 @@ func inlineCostOK(n *ir.CallExpr, caller, callee *ir.Func, bigCaller bool) (bool
10441044
return false, inlineHotMaxBudget
10451045
}
10461046

1047+
if !base.PGOHash.MatchPosWithInfo(n.Pos(), "inline", nil) {
1048+
// De-selected by PGO Hash.
1049+
return false, maxCost
1050+
}
1051+
10471052
if base.Debug.PGODebug > 0 {
10481053
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))
10491054
}

src/cmd/compile/internal/test/pgo_inl_test.go

Lines changed: 69 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package test
66

77
import (
88
"bufio"
9+
"bytes"
910
"fmt"
1011
"internal/profile"
1112
"internal/testenv"
@@ -17,11 +18,7 @@ import (
1718
"testing"
1819
)
1920

20-
// testPGOIntendedInlining tests that specific functions are inlined.
21-
func testPGOIntendedInlining(t *testing.T, dir string) {
22-
testenv.MustHaveGoRun(t)
23-
t.Parallel()
24-
21+
func buildPGOInliningTest(t *testing.T, dir string, flags ...string) []byte {
2522
const pkg = "example.com/pgo/inline"
2623

2724
// Add a go.mod so we have a consistent symbol names in this temp dir.
@@ -32,6 +29,25 @@ go 1.19
3229
t.Fatalf("error writing go.mod: %v", err)
3330
}
3431

32+
exe := filepath.Join(dir, "test.exe")
33+
args := []string{"test", "-c", "-o", exe}
34+
args = append(args, flags...)
35+
cmd := testenv.CleanCmdEnv(testenv.Command(t, testenv.GoToolPath(t), args...))
36+
cmd.Dir = dir
37+
out, err := cmd.CombinedOutput()
38+
if err != nil {
39+
t.Fatalf("build failed: %v, output:\n%s", err, out)
40+
}
41+
return out
42+
}
43+
44+
// testPGOIntendedInlining tests that specific functions are inlined.
45+
func testPGOIntendedInlining(t *testing.T, dir string) {
46+
testenv.MustHaveGoRun(t)
47+
t.Parallel()
48+
49+
const pkg = "example.com/pgo/inline"
50+
3551
want := []string{
3652
"(*BS).NS",
3753
}
@@ -71,25 +87,9 @@ go 1.19
7187
// TODO: maybe adjust the test to work with default threshold.
7288
pprof := filepath.Join(dir, "inline_hot.pprof")
7389
gcflag := fmt.Sprintf("-gcflags=-m -m -pgoprofile=%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90", pprof)
74-
out := filepath.Join(dir, "test.exe")
75-
cmd := testenv.CleanCmdEnv(testenv.Command(t, testenv.GoToolPath(t), "test", "-c", "-o", out, gcflag, "."))
76-
cmd.Dir = dir
77-
78-
pr, pw, err := os.Pipe()
79-
if err != nil {
80-
t.Fatalf("error creating pipe: %v", err)
81-
}
82-
defer pr.Close()
83-
cmd.Stdout = pw
84-
cmd.Stderr = pw
85-
86-
err = cmd.Start()
87-
pw.Close()
88-
if err != nil {
89-
t.Fatalf("error starting go test: %v", err)
90-
}
90+
out := buildPGOInliningTest(t, dir, gcflag)
9191

92-
scanner := bufio.NewScanner(pr)
92+
scanner := bufio.NewScanner(bytes.NewReader(out))
9393
curPkg := ""
9494
canInline := regexp.MustCompile(`: can inline ([^ ]*)`)
9595
haveInlined := regexp.MustCompile(`: inlining call to ([^ ]*)`)
@@ -128,11 +128,8 @@ go 1.19
128128
continue
129129
}
130130
}
131-
if err := cmd.Wait(); err != nil {
132-
t.Fatalf("error running go test: %v", err)
133-
}
134131
if err := scanner.Err(); err != nil {
135-
t.Fatalf("error reading go test output: %v", err)
132+
t.Fatalf("error reading output: %v", err)
136133
}
137134
for fullName, reason := range notInlinedReason {
138135
t.Errorf("%s was not inlined: %s", fullName, reason)
@@ -297,3 +294,48 @@ func copyFile(dst, src string) error {
297294
_, err = io.Copy(d, s)
298295
return err
299296
}
297+
298+
// TestPGOHash tests that PGO optimization decisions can be selected by pgohash.
299+
func TestPGOHash(t *testing.T) {
300+
testenv.MustHaveGoRun(t)
301+
t.Parallel()
302+
303+
wd, err := os.Getwd()
304+
if err != nil {
305+
t.Fatalf("error getting wd: %v", err)
306+
}
307+
srcDir := filepath.Join(wd, "testdata/pgo/inline")
308+
309+
// Copy the module to a scratch location so we can add a go.mod.
310+
dir := t.TempDir()
311+
312+
for _, file := range []string{"inline_hot.go", "inline_hot_test.go", "inline_hot.pprof"} {
313+
if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil {
314+
t.Fatalf("error copying %s: %v", file, err)
315+
}
316+
}
317+
318+
pprof := filepath.Join(dir, "inline_hot.pprof")
319+
gcflag0 := fmt.Sprintf("-gcflags=-pgoprofile=%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90,pgodebug=1,", pprof)
320+
321+
// Check that a hash match allows PGO inlining.
322+
const srcPos = "example.com/pgo/inline/inline_hot.go:81:19"
323+
const hashMatch = "pgohash triggered " + srcPos + " (inline)"
324+
pgoDebugRE := regexp.MustCompile(`hot-budget check allows inlining for call .* at ` + strings.ReplaceAll(srcPos, ".", "\\."))
325+
hash := "v1" // 1 matches srcPos, v for verbose (print source location)
326+
gcflag := gcflag0 + ",pgohash=" + hash
327+
// build with -trimpath so the source location (thus the hash)
328+
// does not depend on the temporary directory path.
329+
out := buildPGOInliningTest(t, dir, gcflag, "-trimpath")
330+
if !bytes.Contains(out, []byte(hashMatch)) || !pgoDebugRE.Match(out) {
331+
t.Errorf("output does not contain expected source line, out:\n%s", out)
332+
}
333+
334+
// Check that a hash mismatch turns off PGO inlining.
335+
hash = "v0" // 0 should not match srcPos
336+
gcflag = gcflag0 + ",pgohash=" + hash
337+
out = buildPGOInliningTest(t, dir, gcflag, "-trimpath")
338+
if bytes.Contains(out, []byte(hashMatch)) || pgoDebugRE.Match(out) {
339+
t.Errorf("output contains unexpected source line, out:\n%s", out)
340+
}
341+
}

0 commit comments

Comments
 (0)