8329531: compiler/c2/irTests/TestIfMinMax.java fails with IRViolationException: There were one or multiple IR rule failures.

[jaskarth]

This patch fixes an issue with the TestIfMinMax IR test where specific random seeds cause the branch probability to be so low that the branch cannot CMove, causing the IR check to fail for min/max reductions. For example, if the first value returned by Random#nextInt was int_min then the branch will be only taken once, which works out to 1/512 => ~0.002. This value is smaller than the CMove threshold 0.01, so it cannot CMove.
I've added sequential values from 1 to 50 and -1 to -50 before inserting random values in the array, so there will be a guaranteed 50 successes and 50 failures for each run. I've also replaced the multiplication by two with an opaque multiplication by one to prevent the randomly generated numbers from becoming larger, like what the test MinMaxRed_Int does.

Thoughts and reviews would be appreciated!

---

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-8329531: compiler/c2/irTests/TestIfMinMax.java fails with IRViolationException: There were one or multiple IR rule failures. (Bug - P4)

Reviewers

• Emanuel Peter (@eme64 - Reviewer)
• Damon Fenacci (@dafedafe - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18734

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8329531: compiler/c2/irTests/TestIfMinMax.java fails with IRViolationException: There were one or multiple IR rule failures.

Reviewed-by: epeter, dfenacci

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

• 3e65d90: 8330820: Remove remnants of operator_new.cpp in build system
• 936a47d: 8330607: Deprecate -XX:+UseEmptySlotsInSupers
• ee7b2e9: 8330051: Small ObjectMonitor spinning code cleanups
• 3e185c7: 8330154: Serial: Remove TenuredSpace::update_for_block
• f889797: 8330463: Rename invalidate() to write_region() in ModRefBarrierSet
• c4f5c51: 8330576: ZYoungCompactionLimit should have range check
• 70acade: 8330339: G1: Move some public methods to private in G1BlockOffsetTable APIs
• 35b30c8: 8330595: Invoke ObjectMethods::bootstrap method exactly
• 3d62bbf: 8330681: Explicit hashCode and equals for java.lang.runtime.SwitchBootstraps$TypePairs
• 5313dcc: 8330733: Generational ZGC: Remove ZBarrier::verify_old_object_live_slow_path
• ... and 258 more: https://git.openjdk.org/jdk/compare/7eb78e332080df3890b66ca04338a4ba69af45eb...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 (@eme64, @dafedafe) 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]

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

• 00: Full (f7641aa9)

[eme64]

Looks reasonable, thanks for the fix!

[dafedafe]

Thanks for fixing this @jaskarth.
Quick question: I noticed that by setting b[i] = 1; now a few tests always set 1 on the right-hand-side of the max/min operations. Could it possibly limit their scope?

[jaskarth]

Thanks for the review @eme64, and thanks for taking a look @dafedafe! I think the scope of the tests shouldn't be limited by this change. The vectorization IR test is the only test that takes the 1s as the right hand argument, and I think functionally the it should behave the same as the true/false ratio will still be roughly equal before and after. Before we were comparing rand_int < rand_int, which is true roughly 50% of the time. Comparing against 1 it should still be true roughly 50% of the time since nextInt() distributes ints across the whole domain.

[jaskarth]

Thanks again for the reviews :)

/integrate

[openjdk]

@jaskarth
Your change (at version f7641aa) is now ready to be sponsored by a Committer.

[TobiHartmann]

/sponsor

[openjdk]

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

• 80b381e: 8329555: Crash in intrinsifying heap-based MemorySegment Vector store/loads
• 7a89555: 8330844: Add aliases for conditional jumps and additional instruction forms for x86
• f60798a: 8329222: java.text.NumberFormat (and subclasses) spec updates
• 2555166: 8329113: Deprecate -XX:+UseNotificationThread
• 09b8809: 8327289: Remove unused PrintMethodFlushingStatistics option
• 9cc163a: 8330178: Clean up non-standard use of /** comments in java.base
• 88a5dce: 8330805: ARM32 build is broken after JDK-8139457
• 7157eea: 8327290: Remove unused notproduct option TraceInvocationCounterOverflow
• b4cea70: 8330693: Generational ZGC: Simplify ZAddress::finalizable_good and ZAddress::mark_good
• 412e306: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
• ... and 291 more: https://git.openjdk.org/jdk/compare/7eb78e332080df3890b66ca04338a4ba69af45eb...master

Your commit was automatically rebased without conflicts.

[openjdk]

@TobiHartmann @jaskarth Pushed as commit 438e643.

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