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.

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

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
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

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

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
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
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
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>
@gopherbot
Copy link

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
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>
@gopherbot
Copy link

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
…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>
Nasfame pushed a commit to golangFame/go that referenced this issue Jun 4, 2023
For golang#59959.
For golang#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>
Nasfame pushed a commit to golangFame/go that referenced this issue Jun 4, 2023
…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 golang#60561
Updates golang#59959

Change-Id: I48449b7d8b329d84151bd3b506b8093c262eb2a3
GitHub-Last-Rev: 2d53c55
GitHub-Pull-Request: golang#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

2 participants