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

8273409: Receiver type narrowed by CCP does not always trigger post-parse call devirtualization #5418

Closed
wants to merge 2 commits into from

Conversation

TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Sep 8, 2021

While working on JDK-8273323, I noticed that even if CCP is able to narrow the receiver type of a virtual call to an exact type, post-parse call devirtualization is not always triggered. The problem is that CCP only adds nodes to the IGVN worklist if their types have been improved:

// If x is a TypeNode, capture any more-precise type permanently into Node
if (t != n->bottom_type()) {
hash_delete(n); // changing bottom type may force a rehash
n->raise_bottom_type(t);
_worklist.push(n); // n re-enters the hash table via the worklist

And in turn, IGVN only adds user nodes to the worklist if the type of the parent node could be further improved. As a result, a CallNode is not always added to the worklist if the type of its receiver node has been improved. Often, we are lucky and macro expansion or another optimization phase after CCP adds the CallNode to the worklist. However, as testDynamicCallWithCCP shows, that's not always the case.

The fix is to explicitly add CallStaticJavaNode and CallDynamicJava to the worklist after CCP to give call devirtualization a chance to run:

Node* CallStaticJavaNode::Ideal(PhaseGVN* phase, bool can_reshape) {
CallGenerator* cg = generator();
if (can_reshape && cg != NULL) {
assert(IncrementalInlineMH, "required");

Node* CallDynamicJavaNode::Ideal(PhaseGVN* phase, bool can_reshape) {
CallGenerator* cg = generator();
if (can_reshape && cg != NULL) {
assert(IncrementalInlineVirtual, "required");

I also considered moving the optimization from Ideal to Value (which in contrast to Ideal is already executed during CCP) but we can't trust receiver types during CCP (because CCP goes from optimistic types to pessimistic ones). Also, the current implementation relies on setting the _generator field which is not possible from Value which is declared const:

// Register for late inlining.
cg->set_callee_method(callee);
phase->C->prepend_late_inline(cg); // MH late inlining prepends to the list, so do the same
set_generator(NULL);

All the newly added tests fail IR verification if post-parse call devirtualization is disabled (-XX:-IncrementalInlineVirtual -XX:-IncrementalInlineMH). Without the fix, testDynamicCallWithCCP always fails.

I filed JDK-8273496 to improve how CCP handles this.

Thanks,
Tobias


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273409: Receiver type narrowed by CCP does not always trigger post-parse call devirtualization

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5418/head:pull/5418
$ git checkout pull/5418

Update a local copy of the PR:
$ git checkout pull/5418
$ git pull https://git.openjdk.java.net/jdk pull/5418/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5418

View PR using the GUI difftool:
$ git pr show -t 5418

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5418.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 8, 2021

👋 Welcome back thartmann! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 8, 2021
@openjdk
Copy link

openjdk bot commented Sep 8, 2021

@TobiHartmann The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 8, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 8, 2021

Webrevs

Copy link

@iwanowww iwanowww left a comment

Looks good.

@openjdk
Copy link

openjdk bot commented Sep 8, 2021

@TobiHartmann This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8273409: Receiver type narrowed by CCP does not always trigger post-parse call devirtualization

Reviewed-by: vlivanov, neliasso

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

  • 7e662e7: 8272413: Incorrect num of element count calculation for vector cast
  • f2f8136: 8265443: IGV: disambiguate groups by emiting additional properties
  • 59c9f75: 8273375: Remove redundant 'new String' calls after concatenation in java.desktop

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 8, 2021
@TobiHartmann
Copy link
Member Author

TobiHartmann commented Sep 8, 2021

Thanks, Vladimir!

Copy link

@neliasso neliasso left a comment

Looks good!

@TobiHartmann
Copy link
Member Author

TobiHartmann commented Sep 8, 2021

Thanks, Nils!

@TobiHartmann
Copy link
Member Author

TobiHartmann commented Sep 9, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Sep 9, 2021

Going to push as commit 4866eaa.
Since your change was applied there have been 25 commits pushed to the master branch:

  • 5ca26cb: 8273433: Enable parallelism in vmTestbase_nsk_sysdict tests
  • f6cc173: 8273539: [PPC64] gtest build error after JDK-8264207
  • 9690df7: 8273476: G1: refine G1CollectedHeap::par_iterate_regions_array_part_from
  • 00e059d: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order
  • aa93111: 8273483: Zero: Clear pending JNI exception check in native method handler
  • 8c16f48: 8273487: Zero: Handle "zero" variant in runtime tests
  • dc33bd8: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
  • 5b1dfe4: 8273439: Fix G1CollectedHeap includes and forward declarations
  • 6eba443: 8273387: remove some unreferenced gtk-related functions
  • 5df2648: 8273218: G1: Rename g1EvacuationInfo to g1EvacInfo
  • ... and 15 more: https://git.openjdk.java.net/jdk/compare/faa942c8ba8ad778b6be20ff6d98a5040a9079e9...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 9, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 9, 2021
@openjdk
Copy link

openjdk bot commented Sep 9, 2021

@TobiHartmann Pushed as commit 4866eaa.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
3 participants