8369045: [lworld] valhalla/valuetypes/WeakReferenceTest.java has an unschedulable graph

[dafedafe]

Issue

A few test failed intermittently with -Xcomp on Mac due to an unschedulable graph in C2:

• valhalla/valuetypes/WeakReferenceTest.java
• valhalla/valuetypes/ProxyTest.java
• valhalla/valuetypes/ObjectNewInstance.java
• valhalla/valuetypes/ObjectMethodsViaCondy.java

Causes

The origin of the issue seems to be coming from strength reduction (process_late_inline_calls_no_inline) where we replace virtual and MH calls with direct calls.
https://github.com/dafedafe/valhalla/blob/75e2dd95df5d847d7d6e35a23016d22705681cf4/src/hotspot/share/opto/compile.cpp#L3072
If the return type of the methods are not loaded, we add a call to runtime's store_inline_type_fields_to_buf right after the actual method call, to save the scalarized return into a oop. This happens first for the caller at parse time and then for the callee when strength-reducing the virtual call to a direct one. The return projections of the inline fields of the call are added to store_inline_type_fields_to_buf and its oop projection is then added as input to the other store_inline_type_fields_to_buf which fundamentally leaves the graph around it in an awkward state.

If this happens in combination with loop unswitching it can lead to a graph not being schedulable, which is what happens in the failure of this issue, where we have:

• a virtual call with a following store_inline_type_fields_to_buf (1) in a loop.
• the loop undergoes unswitching, which creates 2 copies of the body (including a copy of the virtual call). All outputs of the new virtual call are phi-d with the one of the other path as input of the store_inline_type_fields_to_buf.
• the new copy of the virtual call is later replaced with a direct call: the creation of the new direct call adds a store_inline_type_fields_to_buf (2) right after the direct call. All the inline type return values of the call being inlined are removed, so the phis now only have one input and are removed by a later GVN pass.

• this creates an issue later during GCM since the store_inline_type_fields_to_buf (1) call is logically not dominated by any of the 2 sides of the unswitched loop and lands in a separate dominance path of the arguments whose phis have been removed (which are still dominated by the original virtual call).

Solution

The issue happens only when strength-reducing to a direct call and the return type is not loaded. So, there seems to be 2 conceivable fixes: either we avoid strength-reduction if the type is not loaded or we avoid creating the second store_inline_type_fields_to_buf and rewire the output projections of the strength-reduced call to the output of the projections of the original virtual call.
By choosing the first solution we potentially skip a useful optimization (in particular if the return value is always null, which won't trigger the deoptimization). So, the rewiring solution seems to be the best option in this case.

Testing

• repeated testing with failing tests (same conditions)
• replay of failing compilations
• Tier 1-3

Unfortunately creating a regression test that would consistently trigger the specific issue with unloaded return type and strength-reduction to direct call has proven to be unviable.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• JDK-8369045: [lworld] valhalla/valuetypes/WeakReferenceTest.java has an unschedulable graph (Bug - P4)

Reviewers

• Tobias Hartmann (@TobiHartmann - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1768/head:pull/1768
$ git checkout pull/1768

Update a local copy of the PR:
$ git checkout pull/1768
$ git pull https://git.openjdk.org/valhalla.git pull/1768/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1768

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1768.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

@dafedafe 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:

8369045: [lworld] valhalla/valuetypes/WeakReferenceTest.java has an unschedulable graph

Reviewed-by: thartmann

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 5 new commits pushed to the lworld branch:

• 328cfbe: 8372806: [lworld] x64: save bad values instead of rfp and lr above the extension space
• fe500a0: 8367408: [lworld] runtime/cds/appcds/methodHandles/MethodHandlesGeneralTest.java#aot fails
• f682e44: 8368799: [lworld] Heapwalking functions may report array classes several times
• ... and 2 more: https://git.openjdk.org/valhalla/compare/6886e815f01f1e553718460deecf4aefa2b0dad6...lworld

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 lworld branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 08: Full - Incremental (b0c496af)
• 07: Full (ab535b4d)
• 06: Full (b2c399f4)
• 05: Full - Incremental (7790bef4)
• 04: Full - Incremental (32fae207)
• 03: Full - Incremental (e1fe634f)
• 02: Full - Incremental (60aef931)
• 01: Full - Incremental (ccf58125)
• 00: Full (76f2e96f)

[TobiHartmann]

You also need to remove the tests from the problem list again (see #1793).

[openjdk]

⚠️ @dafedafe This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[openjdk]

@dafedafe this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8369045
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

[dafedafe]

You also need to remove the tests from the problem list again (see #1793).

Removed.

[TobiHartmann]

Looks good to me!

[TobiHartmann]

Still good.

[dafedafe]

Thank you for your review @TobiHartmann!

[dafedafe]

/integrate

[openjdk]

Going to push as commit 4ffaf14.
Since your change was applied there have been 5 commits pushed to the lworld branch:

• 328cfbe: 8372806: [lworld] x64: save bad values instead of rfp and lr above the extension space
• fe500a0: 8367408: [lworld] runtime/cds/appcds/methodHandles/MethodHandlesGeneralTest.java#aot fails
• f682e44: 8368799: [lworld] Heapwalking functions may report array classes several times
• ... and 2 more: https://git.openjdk.org/valhalla/compare/6886e815f01f1e553718460deecf4aefa2b0dad6...lworld

Your commit was automatically rebased without conflicts.

[openjdk]

@dafedafe Pushed as commit 4ffaf14.

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