8371558: C2: Missing optimization opportunity in AbsNode::Ideal

[benoitmaillard]

This PR addresses another missed optimization in PhaseIterGVN due to a missing notification to indirect users within PhaseIterGVN::add_users_of_use_to_worklist.

The affected optimization is the transformation of abs(0-x) into abs(x). This transformation is implemented in AbsNode::Ideal.

The bug was found by the fuzzer. At some point during IGVN, we have the following setup:

Phi  ... 
 \  /
 SubI
  |
 AbsI

The Phi node gets folded into a ConI, and we call replace_node(phi, zero), which ends up calling add_users_to_worklist(phi), and add_users_of_use_to_worklist(phi, zero, ...). However the case for this specific notification was missing there, and the AbsI node is never notified (not added to the worklist).

This PR brings the following changes:

• Detect the optimization pattern in add_users_of_use_to_worklist for AbsI, AbsL, AbsF and AbsD
• Add new test TestMissingOptAbsZeroMinusX.java, initially obtained from the fuzzer and then heavily reduced, both with the usual tools and manually. In addition to AbsI, I have also added test cases for AbsF and AbsD, but was not able to reproduce for AbsL despite my best efforts.

Testing

• GitHub Actions
• tier1-4, plus some internal testing

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-8371558: C2: Missing optimization opportunity in AbsNode::Ideal (Bug - P4)

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer) Review applies to 9df620da
• Tobias Hartmann (@TobiHartmann - Reviewer)
• Roberto Castañeda Lozano (@robcasloz - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28237

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28237.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:

8371558: C2: Missing optimization opportunity in AbsNode::Ideal

Reviewed-by: thartmann, rcastanedalo, chagedorn

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 21 new commits pushed to the master branch:

• 1f1f7bb: 8370024: HttpClient: QUIC congestion controller doesn't implement pacing
• 400a83d: 8371625: G1: G1HeapRegion::print_on misalignment
• 76a0732: 8366691: JShell should support a more convenient completion
• ... and 18 more: https://git.openjdk.org/jdk/compare/6e838d6f9a59e4f14d4d1a6d4ff6ea857c0615e9...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.

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

[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.

[mlbridge]

Webrevs

• 01: Full - Incremental (83077987)
• 00: Full (9df620da)

[chhagedorn]

Looks good, thanks!

[TobiHartmann]

Looks good to me otherwise.

[robcasloz]

Looks good!
At some point, we will have to think about some kind of system to enforce notification of indirect users "by construction", or at least make it possible to somehow detect when such notification may be needed.

[benoitmaillard]

Thank you for the reviews! @chhagedorn @TobiHartmann @robcasloz

At some point, we will have to think about some kind of system to enforce notification of indirect users "by construction", or at least make it possible to somehow detect when such notification may be needed.

I agree, this seems quite difficult to sustain over time. But finding a good solution is probably quite hard, as there are different locations in the code where these notifications can happen (add_users_of_use_to_worklist is only one of them).

In the meantime at least we get the opportunity to add more tests (most work in these PRs goes into reproducing these missing optimizations reliably and reducing the tests to the maximum), and this will still be super useful if we ever change the notification mechanism.

/integrate

[openjdk]

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

• 10220ed: 8367013: Add Atomic to package/replace idiom of volatile var plus AtomicAccess:: operations
• 795ec5c: 8370333: hotspot-unit-tests.md specifies wrong directory structure for tests
• 436b335: 8371450: AES performance improvements for key schedule generation
• ... and 35 more: https://git.openjdk.org/jdk/compare/6e838d6f9a59e4f14d4d1a6d4ff6ea857c0615e9...master

Your commit was automatically rebased without conflicts.

[openjdk]

@benoitmaillard Pushed as commit 9d6a61f.

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