cmd/compile/internal/inline: hairyVisitor doesn't properly account for conditionally-inlined PGO calls #59484
Assignees
Labels
Milestone
Comments
prattmic
added
the
NeedsFix
|
Change https://go.dev/cl/483196 mentions this issue: |
github-project-automation
bot
moved this from In Progress
to Done
in Go Compiler / Runtime
yunginnanet
pushed a commit
to yunginnanet/go
that referenced
this issue
Oct 20, 2023
…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 <thanm@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
In
hairVisitor.doNodewe give calls a cost of 57, unless the call is inlineable, in which case we use the cost of the callee, under the assumption that this call will get inlined."Inlineable" here more-or-less just means that
fn.Inl != nil. In a non-PGO build, we only setfn.Inlif the function cost is < 80, making it inlineable ~anywhere.In a PGO build,
fn.Inlis set for any function involved in a hot callsite (e.g.,a->b), but we will only inline at the hot callsite. Any other callsite (e.g.,c->b) will not inline. This makes the assumption incorrect and means that the cold caller (c) will have an artificially inflated cost, potentially making it too expensive to inline.I will send a CL to fix this.
cc @cherrymui @thanm @mdempsky
The text was updated successfully, but these errors were encountered: