Skip to content

cmd/compile/internal/devirtualize: devirtualize methods in other packages if current package has a concrete reference #60565

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
Closed
wants to merge 5 commits into from
Closed

cmd/compile/internal/devirtualize: devirtualize methods in other packages if current package has a concrete reference #60565

wants to merge 5 commits into from
+62 −35

Conversation

thepudds
Copy link
Contributor

@thepudds thepudds commented Jun 1, 2023 •
edited
Loading

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

@thepudds
cmd/compile/internal/devirtualize: devirtualize methods in other pack…
df45919 
…ages if current package has a concrete reference

WIP.
@gopherbot
Copy link
Contributor

This PR (HEAD: df45919) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/500155 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 1: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1: TryBot-Result+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@thepudds
cmd/compile/internal/devirtualize: use typecheck.HaveInlineBody to ch…
91ee96e 
…eck if the inliner has concluded imported func is inlinable
@gopherbot
Copy link
Contributor

This PR (HEAD: 91ee96e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/500155 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 5: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 5:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 5: TryBot-Result+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cuong Manh Le:

Patch Set 5:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Michael Pratt:

Patch Set 5:

(3 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@thepudds
cmd/compile/internal/devirtualize: update PGO devirt test to include …
e3c8fad 
…a method from another package; devirt.pprof not yet updated
@gopherbot
Copy link
Contributor

This PR (HEAD: e3c8fad) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/500155 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from [email protected]:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 8a314f7) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/500155 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 7: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Michael Pratt:

Patch Set 7: Code-Review+2

(5 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 7: TryBot-Result+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 7: Code-Review+2

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 2d53c55) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/500155 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 9: Run-TryBot+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 9:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 9:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 9: Code-Review+2

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 9: TryBot-Result+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/500155.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/500155 has been merged.

@gopherbot gopherbot closed this Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cmd/compile: PGO does not devirtualize methods in another package (log: "no function body")
2 participants
@thepudds @gopherbot