8334629: [BACKOUT] PhaseIdealLoop::conditional_move is too conservative

[merykitty]

Hi,

I cannot explain the regression, comparing the current mainline to JDK-21 reveals a decrease in performance, yet it is only for some combinations of OS-GC and perfasm shows that the hot regions (>99% of execution time) do not contain differences that can explain the results.

Consequently, with the advice of @TobiHartmann , I propose to effectively revert JDK-8319451 for the generation of CMoves inside loops. For those outside, the before-JDK-8319451 probability threshold is 0.001 and the current value is 0.01. I think the current value is more reasonable as evidenced by the benchmark added in JDK-8319451.

Please kindly review, thank you very much.
Quan Anh

---

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-8334629: [BACKOUT] PhaseIdealLoop::conditional_move is too conservative (Sub-task - P3)

Reviewers

• Emanuel Peter (@eme64 - Reviewer) ⚠️ Review applies to caaee62b
• Tobias Hartmann (@TobiHartmann - Reviewer)
• Jasmine Karthikeyan (@jaskarth - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19650

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8334629: [BACKOUT] PhaseIdealLoop::conditional_move is too conservative

Reviewed-by: epeter, thartmann, jkarthikeyan

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

• b2930c5: 8334040: jdk/classfile/CorpusTest.java timed out
• e825ccf: 8332362: Implement os::committed_in_range for MacOS and AIX
• 5ac2149: 8334299: Deprecate LockingMode option, along with LM_LEGACY and LM_MONITOR
• 2e64d15: 8334564: VM startup: fatal error: FLAG_SET_ERGO cannot be used to set an invalid value for NonNMethodCodeHeapSize
• 9d4a4bd: 8324841: PKCS11 tests still skip execution
• ca5a438: 8334571: Extract control dependency rewiring out of PhaseIdealLoop::dominated_by() into separate method
• 05ff318: 8334594: Generational ZGC: Deadlock after OopMap rewrites in 8331572
• 05a63d8: 8334489: Add function os::used_memory
• edf7f05: 8334083: C2 SuperWord: TestCompatibleUseDefTypeSize.java fails with -XX:+AlignVector after JDK-8325155
• 13dce29: 8334560: [PPC64]: postalloc_expand_java_dynamic_call_sched does not copy all fields
• ... and 130 more: https://git.openjdk.org/jdk/compare/93f3918ee16fd4360f1d57c379a4bdc4baa88f2b...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]

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

• 02: Full - Incremental (b8819793)
• 01: Full - Incremental (caaee62b)
• 00: Full (0c0c9780)

[merykitty]

@TobiHartmann

Can you verify if the regression is solved with this patch.

Also, I decided to not include your benchmark. The reason is that it is actually similar to that added by JDK-8319451. For such a small array, I believe the branch predictor can remember the whole sequence, which results in perfect prediction all the time, explaining the results. I recall that when making that benchmark, my CPU could remember an array of 1000 elements, which led to me using a huge array of 1 million.

Thank you very much.

[TobiHartmann]

Thanks, Quan Anh. On second thought, I think it's best to cleanly backout JDK-8319451 for now and file a REDO to get rid of the dependency on BlockLayoutMinDiamondPercentage. I think this would actually require a CSR though because BlockLayoutMinDiamondPercentage is a product flag and we chang it's behavior.

[openjdk]

@merykitty Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[merykitty]

@TobiHartmann Thanks for your suggestion, I have changed this to a backout, I need to also rename the issue, right?

[eme64]

@TobiHartmann @merykitty
Usually, you file a [REDO], and make the [BACKOUT] a subtask. Example:
https://bugs.openjdk.org/browse/JDK-8323727

[merykitty]

@eme64 Thanks for the info, what if we only want to backout the change without the intention of doing a REDO?

[TobiHartmann]

The OpenJDK Guide has all the details, see "Alternative 3":
https://openjdk.org/guide/#backing-out-a-change

I think creating a REDO is fine though, since we definitely want to get rid of the dependency on BlockLayoutMinDiamondPercentage at some point. Feel free to leave it unassigned though.

[merykitty]

@TobiHartmann Thanks for your suggestion, I have made relevant issues and changed the title of this PR, can you please review it?

[eme64]

Looks good, it is the exact backout of https://github.com/openjdk/jdk/pull/16524/files.

I guess it is a shame that you are also backing out the microbenchmark, but you can easily push that separately or in your REDO.

[eme64]

Aha, I just saw that there are apparently failures on GitHub actions:

Failed IR Rules (2) of Methods (2)
----------------------------------
1) Method "public java.lang.Object[] compiler.c2.irTests.TestIfMinMax.testMaxIntReduction(int[],int[])" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={"sse4.1", "true", "asimd", "true"}, counts={"_#MAX_REDUCTION_V#_", "> 0"}, failOn={}, applyIfPlatform={}, applyIfPlatformOr={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={"SuperWordReductions", "true"}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\d+(\s){2}(MaxReductionV.*)+(\s){2}===.*)"
           - Failed comparison: [found] 0 > 0 [given]
           - No nodes matched!

2) Method "public java.lang.Object[] compiler.c2.irTests.TestIfMinMax.testMinIntReduction(int[],int[])" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={"sse4.1", "true", "asimd", "true"}, counts={"_#MIN_REDUCTION_V#_", "> 0"}, failOn={}, applyIfPlatform={}, applyIfPlatformOr={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={"SuperWordReductions", "true"}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\d+(\s){2}(MinReductionV.*)+(\s){2}===.*)"
           - Failed comparison: [found] 0 > 0 [given]
           - No nodes matched!

Could these be related?

[eme64]

It is possible that the test that @jaskarth added in the meantime relied on your change:
9f920b9

https://github.com/openjdk/jdk/blame/master/test/hotspot/jtreg/compiler/c2/irTests/TestIfMinMax.java

[eme64]

I'll run testing from our side, please ask for the results before you integrate.

[jaskarth]

Ah yep, looking at the log the main IfNode has P=0.891189, so it doesn't succeed the new check prob > 0.18 && prob < 0.82. I think this is fundementally the same problem as #18734, and increasing the number of predetermined elements (here) should fix this. If it's important to get this patch in quickly though feel free to problemlist the test or disable the IR verification, I can take a look at it soon.

[merykitty]

@jaskarth Thanks for your analysis, I added the test to the problem list and created a relevant issue for it.

[TobiHartmann]

Looks good to me too.

[jaskarth]

Thanks for creating the issue! I've assigned myself to it. The patch looks good to me as well.

[eme64]

@merykitty testing looks good (only failed for compiler/c2/irTests/TestIfMinMax.java but you now problem-listed it).

[merykitty]

@eme64 @TobiHartmann @jaskarth Thanks a lot for your reviews.
/integrate

[openjdk]

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

• 7429c37: 8334598: Default classlist in JDK is not deterministic after JDK-8293980
• 9c89f08: 8334421: assert(!oldbox->is_unbalanced()) failed: this should not be called for unbalanced region
• 57f8b91: 8333658: NMT: Use an allocator with 4-byte pointers to save memory in NativeCallStackStorage
• 6c67933: 8334899: Test runtime/cds/appcds/javaldr/ExceptionDuringDumpAtObjectsInitPhase.java failed after JDK-8306580
• cae94b2: 8334397: RISC-V: verify perf of ReverseBytesS/US
• 75a2afa: 8248981: Specify list of standard message digest and mgf algorithms for RSASSA-PSS signature
• baafa66: 8334287: Man page update for jstatd deprecation
• c30e040: 8331911: Reconsider locking for recently disarmed nmethods
• 974dca8: 8334223: Make Arena MEMFLAGs immutable
• e527e1c: 8334580: Deprecate no-arg constructor BasicSliderUI() for removal
• ... and 144 more: https://git.openjdk.org/jdk/compare/93f3918ee16fd4360f1d57c379a4bdc4baa88f2b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@merykitty Pushed as commit 933eaba.

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

[TobiHartmann]

/backport jdk:jdk23

[openjdk]

@TobiHartmann the backport was successfully created on the branch backport-TobiHartmann-933eabab-jdk23 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk23, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 933eabab from the openjdk/jdk repository.

The commit being backported was authored by Quan Anh Mai on 25 Jun 2024 and was reviewed by Emanuel Peter, Tobias Hartmann and Jasmine Karthikeyan.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-TobiHartmann-933eabab-jdk23:backport-TobiHartmann-933eabab-jdk23
$ git checkout backport-TobiHartmann-933eabab-jdk23
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-TobiHartmann-933eabab-jdk23

⚠️ @TobiHartmann You are not yet a collaborator in my fork openjdk-bots/jdk. An invite will be sent out and you need to accept it before you can proceed.