8359603: Missed optimization in PhaseIterGVN for redundant ConvX2Y->ConvY2X->ConvX2Y sequences due to missing notification in PhaseIterGVN::add_users_of_use_to_worklist

[benoitmaillard]

This PR addresses a missed optimization in PhaseIterGVN due to the lack of change notification to indirect users within PhaseIterGVN::add_users_of_use_to_worklist (again). This is similar to JDK-8361700.

The optimization in question is the removal of redundant ConvX2Y->ConvY2X->ConvX2Y sequences (where X and Y are primitive number types), which get replaced by a single ConvX2Y as an identity optimization. This missing optimization was originally reported only for ConvD2LNode, but it turns out that other conversion nodes have analog optimization patterns. After manual inspection of identity optimizations in convertnode.cpp, I was able to reproduce missing optimizations for the following conversion sequences:

• ConvD2L->ConvL2D->ConvD2L
• ConvF2I->ConvI2F->ConvF2I
• ConvF2L->ConvL2F->ConvF2L
• ConvI2F->ConvF2I->ConvI2F

Similar optimization patterns exist for additional conversion nodes. However, it is not clear if these nodes are subject to the same missed optimization issue. Further investigation may be needed, as I was unable to reproduce such cases with simple tests.

This is again a case where an optimization depends on the input of its input. Currently, PhaseIterGVN::add_users_of_use_to_worklist contains specific logic to handle similar dependencies for other cases, but this specific scenario is not addressed. The proposed fix adds the necessary logic in add_users_of_use_to_worklist to ensure proper notification for this optimization pattern.

Testing

• GitHub Actions
• tier1-3, plus some internal testing
• Added test from the fuzzer, and tests for other sequences (manually derived from the original one)

Thank you for reviewing!

---

Progress

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

Issue

• JDK-8359603: Missed optimization in PhaseIterGVN for redundant ConvX2Y->ConvY2X->ConvX2Y sequences due to missing notification in PhaseIterGVN::add_users_of_use_to_worklist (Bug - P4)

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer)
• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26368

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26368.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back bmaillard! 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]

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

8359603: Missed optimization in PhaseIterGVN for redundant ConvX2Y->ConvY2X->ConvX2Y sequences due to missing notification in PhaseIterGVN::add_users_of_use_to_worklist

Reviewed-by: chagedorn, 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 88 new commits pushed to the master branch:

• 4669005: 8361912: ThreadsListHandle::cv_internal_thread_to_JavaThread does not deal with a virtual thread's carrier thread
• 3e4e5dd: 8364081: Shenandoah & GenShen logging improvement
• ea0b49c: 8362979: C2 fails with unexpected node in SuperWord truncation: CmpLTMask, RoundF
• ... and 85 more: https://git.openjdk.org/jdk/compare/62a58062e5f3d0a723608d98d2412ea779f73897...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@chhagedorn, @TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@benoitmaillard 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]

@benoitmaillard this pull request can not be integrated into master 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-8359603
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[mlbridge]

Webrevs

• 03: Full - Incremental (2e5efdcc)
• 02: Full - Incremental (10f79866)
• 01: Full - Incremental (0b3244fd)
• 00: Full (9c49b040)

[chhagedorn]

Two minor comments, otherwise, it looks good to me!

[chhagedorn]

Another thought: Since this is an incomplete list of variations (especially missing, for example, the I2D version while the I2F version is here), should we leave a comment about not being able to trigger issues with the other versions? Otherwise, it could suggest that it was just forgotten.

[benoitmaillard]

The notification issue that is solved here only happens with optimizations that have this chain pattern with three nodes (checking the input of the input) and this is specific to conversions that have a loss of precision. ConvI2D is not here because the ConvI2D->ConvD2I gets optimized to a NOP already (ConvD2INode::Identity). But there are also chains for which there is a known optimization and for which I was not able to trigger a missed optimization, so it would make sense to have mention this in any case.

[benoitmaillard]

I have added a short note, let me know what you think!

[TobiHartmann]

Looks good to me!

[chhagedorn]

Update looks good, thanks!

[benoitmaillard]

/integrate

[openjdk]

@benoitmaillard
Your change (at version 2e5efdc) is now ready to be sponsored by a Committer.

[chhagedorn]

/sponsor

[openjdk]

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

• 4669005: 8361912: ThreadsListHandle::cv_internal_thread_to_JavaThread does not deal with a virtual thread's carrier thread
• 3e4e5dd: 8364081: Shenandoah & GenShen logging improvement
• ea0b49c: 8362979: C2 fails with unexpected node in SuperWord truncation: CmpLTMask, RoundF
• ... and 85 more: https://git.openjdk.org/jdk/compare/62a58062e5f3d0a723608d98d2412ea779f73897...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn @benoitmaillard Pushed as commit 2829741.

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