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

[Emanuel Peter]

On Tue, 11 Jun 2024 17:35:32 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> 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 `CMove`s 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
>
> Quan Anh Mai has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   Revert "8319451: PhaseIdealLoop::conditional_move is too conservative"
>   
>   This reverts commit ac968c36d7cc2e13270d28c9310178f6b654d7dc.

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.

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?

It is possible that the test that @jaskarth added in the meantime relied on your change:
https://github.com/openjdk/jdk/commit/9f920b9bbf8a64e2c2db085cf3da30db37c0d1bc

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

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

-------------

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19650#pullrequestreview-2130426340
PR Comment: https://git.openjdk.org/jdk/pull/19650#issuecomment-2180688444
PR Comment: https://git.openjdk.org/jdk/pull/19650#issuecomment-2180691690
PR Comment: https://git.openjdk.org/jdk/pull/19650#issuecomment-2180693813