cmd/compile: synchronize inlinability logic between hairyVisitor and mkinlcall

When computing function cost, hairyVisitor.doNode has two primary cases
for determining the cost of a call inside the function:

* Normal calls are simply cost 57.
* Calls that can be inlined have the cost of the inlined function body,
  since that body will end up in this function.

Determining which "calls can be inlined" is where this breaks down.
doNode simply assumes that any function with `fn.Inl != nil` will get
inlined. However, this are more complex in mkinlcall, which has a
variety of cases where it may not inline.

For standard builds, most of these reasons are fairly rare (recursive
calls, calls to runtime functions in instrumented builds, etc), so this
simplification isn't too build.

However, for PGO builds, any function involved in at least one inlinable
hot callsite will have `fn.Inl != nil`, even though mkinlcall will only
inline at the hot callsites. As a result, cold functions calling hot
functions will use the (potentially very large) hot function inline body
cost in their call budget. This could make these functions too expensive
to inline even though they won't actually inline the hot function.

Handle this case plus the other inlinability cases (recursive calls,
etc) by consolidating mkinlcall's inlinability logic into
canInlineCallExpr, which is shared by doNode.

mkinlcall and doNode now have identical logic, except for one case: we
check for recursive cycles via inlined functions by looking at the
inline tree. Since we haven't actually done any inlining yet when in
doNode, we will miss those cases.

This CL doesn't change any inlining decisions in a standard build of the
compiler.

In the new inliner, the inlining decision is also based on the call
site, so this synchronization is also helpful.

Fixes golang#59484

Change-Id: I6ace66e37d50526535972215497ef75cd71f8b9d
Reviewed-on: https://go-review.googlesource.com/c/go/+/483196
Reviewed-by: Than McIntosh <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>

Author: prattmic

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

@@ -361,6 +361,7 @@ func CanInline(fn *ir.Func, profile *pgo.Profile) {
 
 	visitor := hairyVisitor{
 		curFunc:       fn,
+		isBigFunc:     isBigFunc(fn),
 		budget:        budget,
 		maxBudget:     budget,
 		extraCallCost: cc,
@@ -499,6 +500,7 @@ func canDelayResults(fn *ir.Func) bool {
 type hairyVisitor struct {
 	// This is needed to access the current caller in the doNode function.
 	curFunc       *ir.Func
+	isBigFunc     bool
 	budget        int32
 	maxBudget     int32
 	reason        string
@@ -600,41 +602,29 @@ opSwitch:
 			break // treat like any other node, that is, cost of 1
 		}
 
-		// Determine if the callee edge is for an inlinable hot callee or not.
-		if v.profile != nil && v.curFunc != nil {
-			if fn := inlCallee(v.curFunc, n.Fun, v.profile); fn != nil && typecheck.HaveInlineBody(fn) {
-				lineOffset := pgo.NodeLineOffset(n, fn)
-				csi := pgo.CallSiteInfo{LineOffset: lineOffset, Caller: v.curFunc}
-				if _, o := candHotEdgeMap[csi]; o {
-					if base.Debug.PGODebug > 0 {
-						fmt.Printf("hot-callsite identified at line=%v for func=%v\n", ir.Line(n), ir.PkgFuncName(v.curFunc))
-					}
-				}
-			}
-		}
-
 		if ir.IsIntrinsicCall(n) {
 			// Treat like any other node.
 			break
 		}
 
-		if fn := inlCallee(v.curFunc, n.Fun, v.profile); fn != nil && typecheck.HaveInlineBody(fn) {
-			// In the existing inliner, it makes sense to use fn.Inl.Cost
-			// here due to the fact that an "inline F everywhere if F inlinable"
-			// strategy is used. With the new inliner, however, it is not
-			// a given that we'll inline a specific callsite -- it depends
-			// on what score we assign to the callsite. For now, use the
-			// computed cost if lower than the call cost, otherwise
-			// use call cost (we can eventually do away with this when
-			// we move to the "min-heap of callsites" scheme.
-			if !goexperiment.NewInliner {
-				v.budget -= fn.Inl.Cost
+		if callee := inlCallee(v.curFunc, n.Fun, v.profile); callee != nil && typecheck.HaveInlineBody(callee) {
+			// Check whether we'd actually inline this call. Set
+			// log == false since we aren't actually doing inlining
+			// yet.
+			if canInlineCallExpr(v.curFunc, n, callee, v.isBigFunc, false) {
+				// mkinlcall would inline this call [1], so use
+				// the cost of the inline body as the cost of
+				// the call, as that is what will actually
+				// appear in the code.
+				//
+				// [1] This is almost a perfect match to the
+				// mkinlcall logic, except that
+				// canInlineCallExpr considers inlining cycles
+				// by looking at what has already been inlined.
+				// Since we haven't done any inlining yet we
+				// will miss those.
+				v.budget -= callee.Inl.Cost
 				break
-			} else {
-				if fn.Inl.Cost < inlineExtraCallCost {
-					v.budget -= fn.Inl.Cost
-					break
-				}
 			}
 		}
 
@@ -1056,54 +1046,59 @@ func inlineCostOK(n *ir.CallExpr, caller, callee *ir.Func, bigCaller bool) (bool
 	return true, 0
 }
 
-// If n is a OCALLFUNC node, and fn is an ONAME node for a
-// function with an inlinable body, return an OINLCALL node that can replace n.
-// The returned node's Ninit has the parameter assignments, the Nbody is the
-// inlined function body, and (List, Rlist) contain the (input, output)
-// parameters.
-// The result of mkinlcall MUST be assigned back to n, e.g.
+// canInlineCallsite returns true if the call n from caller to callee can be
+// inlined. bigCaller indicates that caller is a big function. log indicates
+// that the 'cannot inline' reason should be logged.
 //
-//	n.Left = mkinlcall(n.Left, fn, isddd)
-func mkinlcall(callerfn *ir.Func, n *ir.CallExpr, fn *ir.Func, bigCaller bool, inlCalls *[]*ir.InlinedCallExpr) ir.Node {
-	if fn.Inl == nil {
-		if logopt.Enabled() {
+// Preconditions: CanInline(callee) has already been called.
+func canInlineCallExpr(callerfn *ir.Func, n *ir.CallExpr, callee *ir.Func, bigCaller bool, log bool) bool {
+	if callee.Inl == nil {
+		// callee is never inlinable.
+		if log && logopt.Enabled() {
 			logopt.LogOpt(n.Pos(), "cannotInlineCall", "inline", ir.FuncName(callerfn),
-				fmt.Sprintf("%s cannot be inlined", ir.PkgFuncName(fn)))
+				fmt.Sprintf("%s cannot be inlined", ir.PkgFuncName(callee)))
 		}
-		return n
+		return false
 	}
 
-	if ok, maxCost := inlineCostOK(n, callerfn, fn, bigCaller); !ok {
-		if logopt.Enabled() {
+	if ok, maxCost := inlineCostOK(n, callerfn, callee, bigCaller); !ok {
+		// callee cost too high for this call site.
+		if log && logopt.Enabled() {
 			logopt.LogOpt(n.Pos(), "cannotInlineCall", "inline", ir.FuncName(callerfn),
-				fmt.Sprintf("cost %d of %s exceeds max caller cost %d", fn.Inl.Cost, ir.PkgFuncName(fn), maxCost))
+				fmt.Sprintf("cost %d of %s exceeds max caller cost %d", callee.Inl.Cost, ir.PkgFuncName(callee), maxCost))
 		}
-		return n
+		return false
 	}
 
-	if fn == callerfn {
+	if callee == callerfn {
 		// Can't recursively inline a function into itself.
-		if logopt.Enabled() {
+		if log && logopt.Enabled() {
 			logopt.LogOpt(n.Pos(), "cannotInlineCall", "inline", fmt.Sprintf("recursive call to %s", ir.FuncName(callerfn)))
 		}
-		return n
+		return false
 	}
 
-	if base.Flag.Cfg.Instrumenting && types.IsNoInstrumentPkg(fn.Sym().Pkg) {
+	if base.Flag.Cfg.Instrumenting && types.IsNoInstrumentPkg(callee.Sym().Pkg) {
 		// Runtime package must not be instrumented.
 		// Instrument skips runtime package. However, some runtime code can be
 		// inlined into other packages and instrumented there. To avoid this,
 		// we disable inlining of runtime functions when instrumenting.
 		// The example that we observed is inlining of LockOSThread,
 		// which lead to false race reports on m contents.
-		return n
-	}
-	if base.Flag.Race && types.IsNoRacePkg(fn.Sym().Pkg) {
-		return n
+		if log && logopt.Enabled() {
+			logopt.LogOpt(n.Pos(), "cannotInlineCall", "inline", ir.FuncName(callerfn),
+				fmt.Sprintf("call to runtime function %s in instrumented build", ir.PkgFuncName(callee)))
+		}
+		return false
 	}
 
-	parent := base.Ctxt.PosTable.Pos(n.Pos()).Base().InliningIndex()
-	sym := fn.Linksym()
+	if base.Flag.Race && types.IsNoRacePkg(callee.Sym().Pkg) {
+		if log && logopt.Enabled() {
+			logopt.LogOpt(n.Pos(), "cannotInlineCall", "inline", ir.FuncName(callerfn),
+				fmt.Sprintf(`call to into "no-race" package function %s in race build`, ir.PkgFuncName(callee)))
+		}
+		return false
+	}
 
 	// Check if we've already inlined this function at this particular
 	// call site, in order to stop inlining when we reach the beginning
@@ -1112,17 +1107,42 @@ func mkinlcall(callerfn *ir.Func, n *ir.CallExpr, fn *ir.Func, bigCaller bool, i
 	// many functions. Most likely, the inlining will stop before we
 	// even hit the beginning of the cycle again, but this catches the
 	// unusual case.
+	parent := base.Ctxt.PosTable.Pos(n.Pos()).Base().InliningIndex()
+	sym := callee.Linksym()
 	for inlIndex := parent; inlIndex >= 0; inlIndex = base.Ctxt.InlTree.Parent(inlIndex) {
 		if base.Ctxt.InlTree.InlinedFunction(inlIndex) == sym {
-			if base.Flag.LowerM > 1 {
-				fmt.Printf("%v: cannot inline %v into %v: repeated recursive cycle\n", ir.Line(n), fn, ir.FuncName(callerfn))
+			if log {
+				if base.Flag.LowerM > 1 {
+					fmt.Printf("%v: cannot inline %v into %v: repeated recursive cycle\n", ir.Line(n), callee, ir.FuncName(callerfn))
+				}
+				if logopt.Enabled() {
+					logopt.LogOpt(n.Pos(), "cannotInlineCall", "inline", ir.FuncName(callerfn),
+						fmt.Sprintf("repeated recursive cycle to %s", ir.PkgFuncName(callee)))
+				}
 			}
-			return n
+			return false
 		}
 	}
 
+	return true
+}
+
+// If n is a OCALLFUNC node, and fn is an ONAME node for a
+// function with an inlinable body, return an OINLCALL node that can replace n.
+// The returned node's Ninit has the parameter assignments, the Nbody is the
+// inlined function body, and (List, Rlist) contain the (input, output)
+// parameters.
+// The result of mkinlcall MUST be assigned back to n, e.g.
+//
+//	n.Left = mkinlcall(n.Left, fn, isddd)
+func mkinlcall(callerfn *ir.Func, n *ir.CallExpr, fn *ir.Func, bigCaller bool, inlCalls *[]*ir.InlinedCallExpr) ir.Node {
+	if !canInlineCallExpr(callerfn, n, fn, bigCaller, true) {
+		return n
+	}
 	typecheck.AssertFixedCall(n)
 
+	parent := base.Ctxt.PosTable.Pos(n.Pos()).Base().InliningIndex()
+	sym := fn.Linksym()
 	inlIndex := base.Ctxt.InlTree.Add(parent, n.Pos(), sym, ir.FuncName(fn))
 
 	closureInitLSym := func(n *ir.CallExpr, fn *ir.Func) {