Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/compile: PGO-driven indirect call specialization #59959

Closed
prattmic opened this issue May 3, 2023 · 7 comments
Closed

cmd/compile: PGO-driven indirect call specialization #59959

prattmic opened this issue May 3, 2023 · 7 comments
Assignees
@prattmic
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@prattmic
Copy link
Member

prattmic commented May 3, 2023

For instance, adding a conditional direct call to the concrete type of the hottest callee of an interface call.

Converting:

iface.Bar()

to

t, ok := iface.(Concrete)
if ok {
	t.Bar()
} else {
	iface.Bar()
}

This can enable inlining of the hot path. It won't affect escape analysis because the fallback path still escapes.

cc @cherrymui @aclements @rajbarik @jinlin-bayarea
@prattmic prattmic added the NeedsFix The path to resolution is known, but the work has not been done. label May 3, 2023
@prattmic prattmic added this to the Go1.21 milestone May 3, 2023
@prattmic prattmic self-assigned this May 3, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 3, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/492436 mentions this issue: WIP: cmd/compile: enable PGO-driven call specialization

@mdempsky mdempsky moved this to In Progress in Go Compiler / Runtime May 10, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/494717 mentions this issue: DO NOT SUBMIT: cmd/pgo: specialization CallStat analysis tool

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/494716 mentions this issue: cmd/compile: replace -d=pgoinline with -d=pgodebug

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/494959 mentions this issue: cmd/compile/internal/typecheck: remove base.Errorf from Assignop

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/495915 mentions this issue: cmd/compile/internal/typecheck: export Implements

gopherbot pushed a commit that referenced this issue May 22, 2023
@prattmic @gopherbot
cmd/compile/internal/typecheck: remove base.Errorf from Assignop
374bd6a 
The documentation for Assignop specifies that if the assignment is not
valid, the reason for the failure is returned via a reason string
without failing the build.

A few cases in Assignop1 -> implements -> ifacelookdot directly call
base.Errorf rather than plumbing through the reason string as they
should. Drop these calls. Since error messages are mostly unreachable
here (it only applies to generated code), don't maintain them and allow
them to just fallthrough to the generic "missing method" message.

This is important for PGO specialization, which opportunistically checks
if candidate interface call targets implement the interface. Many of
these will fail, which should not break the build.

For #59959.

Change-Id: I1891ca0ebebc1c1f51a0d0285035bbe8753036bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/494959
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue May 22, 2023
@prattmic @gopherbot
cmd/compile/internal/typecheck: export Implements
13e9696 
Provide an exported version of implements to easily check if a type
implements an interface. This will be use for PGO devirtualization.

Even within the package, other callers can make use of this simpler API
to reduce duplication.

For #59959.

Change-Id: If4eb86f197ca32abc7634561e36498a247b5070f
Reviewed-on: https://go-review.googlesource.com/c/go/+/495915
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue May 22, 2023
@prattmic @gopherbot
cmd/compile: replace -d=pgoinline with -d=pgodebug
7c2cd0b 
We will soon have PGO specialization. It doesn't make sense for the
debug flag to have inline in the name, so rename it to pgodebug.

pgoinline is now a flag that can be used to disable PGO inlining.
Devirtualization will have a similar debug flag.

For #59959.

Change-Id: I9770ff1f0d132dfa3cd417018a887a1bd5555bba
Reviewed-on: https://go-review.googlesource.com/c/go/+/494716
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime May 22, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/498263 mentions this issue: doc: release notes for PGO devirtualization

gopherbot pushed a commit that referenced this issue Jun 1, 2023
@prattmic
doc/go1.21: document PGO GA and devirtualization
65dcdde 
For #59959.
For #58645.

Change-Id: I574153ef2fd61a5e90ec281fca065c42fce22cc1
Reviewed-on: https://go-review.googlesource.com/c/go/+/498263
Reviewed-by: Eli Bendersky <eliben@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This was referenced Jun 1, 2023
Closed
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/500155 mentions this issue: cmd/compile/internal/devirtualize: devirtualize methods in other packages if current package has a concrete reference

gopherbot pushed a commit that referenced this issue Jun 3, 2023
@thepudds @cherrymui
cmd/compile/internal/devirtualize: devirtualize methods in other pack…
3637132 
…ages if current package has a concrete reference

The new PGO-driven indirect call specialization from CL 492436
in theory should allow for devirtualization on methods
in another package when those methods are directly referenced
in the current package.

However, inline.InlineImpossible was checking for a zero-length
fn.Body and would cause devirtualization to fail
with a debug log message like:

 "should not PGO devirtualize (*Speaker1).Speak: no function body"

Previously, the logic in inline.InlineImpossible was only
called on local functions, but with PGO-based devirtualization,
it can now be called on imported functions, where inlinable
imported functions will have a zero-length fn.Body but a
non-nil fn.Inl.

We update inline.InlineImpossible to handle imported functions
by adding a call to typecheck.HaveInlineBody in the check
that was previously failing.

For the test, we need to have a hopefully temporary workaround
of adding explicit references to the callees in another package
for devirtualization to work. CL 497175 or similar should
enable removing this workaround.

Fixes #60561
Updates #59959

Change-Id: I48449b7d8b329d84151bd3b506b8093c262eb2a3
GitHub-Last-Rev: 2d53c55
GitHub-Pull-Request: #60565
Reviewed-on: https://go-review.googlesource.com/c/go/+/500155
Run-TryBot: thepudds <thepudds1460@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@prattmic prattmic

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
2 participants
@prattmic @gopherbot