8373134: C2: Min/Max users of Min/Max uses should be enqueued for GVN

[galderz]

Min/Max users of Min/Max uses need to be enqueued respectively to the GVN worklist to see if further optimizations can be applied. Without this, there are cases where additional potential ideal/identity optimizations are not applied. I need this fix to test min/max reassociation implementation with IR tests reliably.

Aside from the fix itself, I've refactored MaxNode to MinMaxNode and added a is_MinMax node query to simplify the fix.

I have also removed the Min/Max exceptions in PhaseIterGVN::verify_Identity_for since this fix fixes compiler/codegen/TestBooleanVect.java with -XX:VerifyIterativeGVN=1110.

To test this I've created a template framework test that validates the fix. I have tested with all Min/Max combinations including Float16, which I've verified with Intel SDE. Float16 does not use Argument.NUMBER_42 because there's no support for it yet, see JDK-8373977.

During development I noticed that the test only failed when the test had b, a parameters in that order, so I added tests for both cases as a, b and b, a so that all possible orders are covered and they don't slip in the future.

I've run tier1-3 tests on linux/x64 successfully.

---

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-8373134: C2: Min/Max users of Min/Max uses should be enqueued for GVN (Enhancement - P4)

Reviewers

• Benoît Maillard (@benoitmaillard - Committer) Review applies to 7229e345
• Dean Long (@dean-long - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28895

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8373134: C2: Min/Max users of Min/Max uses should be enqueued for GVN

Reviewed-by: dlong, bmaillard

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

• 805866b: 8372040: Remove Prefetch header vs inline header separation
• 663a083: 8374219: Fix issues in jpackage's Executor class
• f5fa9e4: 8374745: Test vmTestbase/nsk/monitoring/GarbageCollectorMXBean/CollectionCounters/CollectionCounters004/TestDescription.java failed
• ... and 27 more: https://git.openjdk.org/jdk/compare/067fd3cb2fa6a4a0484a922df8efbde03325ad3d...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 (@benoitmaillard, @eme64, @dean-long) 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]

@galderz 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

• 07: Full - Incremental (2c0b0e43)
• 06: Full - Incremental (fe71e03f)
• 05: Full (7229e345)
• 04: Full - Incremental (e2579bb1)
• 03: Full - Incremental (09fba3fb)
• 02: Full - Incremental (1cc8a021)
• 01: Full (4221b67d)
• 00: Full (d06ff3ea)

[dean-long]

So there are no Min(Max()) or Max(Min()) patterns we need to worry about? I was expecting this line to be

if (u->is_MinMax()) {

[galderz]

Good question. There could be some patterns but I couldn't think of any when I was working on this, so I limited it to the patterns that I knew for sure required this, e.g. Max(Max()), Min(Min()).

[benoitmaillard]

I took a quick look and I didn't find anything obvious.

[galderz]

There's an issue with the Float16 incubator API access that I did not notice in local testing, I'm looking into it

[galderz]

The module issue should be fixed now

[benoitmaillard]

Looks good to me, thanks for making this change. I like the move from MaxNode to MinMaxNode, I always thought the naming was a bit confusing.

I have also kicked off internal testing and will come back with the results once done.

[eme64]

Looks great, thanks for working on this Galder!