RFR: 8352504: RISC-V: implement and enable CMoveI/L [v7]

[Fei Yang]

On Thu, 10 Apr 2025 12:08:29 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you help to review this patch?
>> On riscv, CMoveI/L already were implemented, but there are some gap:
>> 1. CMoveI/L does not support comparison with float/double, corresponding tests are not turn on either.
>> 2. Some optimization of C2 is not turned on, e.g. `Phi -> CMove -> min_max`.
>> 3. lack of some corresponding performance tests.
>> 
>> Also there are some issue with current Zicond:
>> 1. UseZicond is turned on automatically by hwprobe, but jmh tests show that it's not always bring benefit, in some situation it even brings regression, the reason is the generated code by Zicond is much larger than branch version, in particular when it's in a loop and unrolled.
>> 
>> This patch on riscv is to:
>> 1. add CMoveI/L comparing float/double, and corresponding tests,
>> 2. enable more C2 optimization,
>> 3. add more benchmark tests,
>> 4. turn off UseZicond by default.
>> 
>> Thanks!
>> 
>> ## Performance
>> 
>> ### MinMax
>> test data
>> <google-sheets-html-origin style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;">
>> Benchmark | Mode | Cnt | Score - master | Score - master+UseZbb | Score - -master+UseZicond | Score - master+UseZicond+UseZbb | Score - cmovei | Score - cmovei+UseZbb | Score - cmovei+UseZicond | Score - cmovei+UseZicond+UseZbb | Error | Units | Opt (master/cmovei)
>> -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
>> o.o.b.vm.compiler.IfMinMax.testReductionInt | avgt | 40 | 17152.075 | 17216.592 | 17272.493 | 17296.89 | 17127.844 | 17036.605 | 17299.333 | 17250.566 | 73.179 | ns/op | 1.001
>> o.o.b.vm.compiler.IfMinMax.testReductionLong | avgt | 40 | 19770.828 | 19967.578 | 20268.905 | 20166.165 | 20065.552 | 20059.095 | 20161.914 | 20151.295 | 131.428 | ns/op | 0.985
>> o.o.b.vm.compiler.IfMinMax.testSingleInt | avgt | 40 | 114.734 | 114.402 | 114.887 | 114.384 | 114.4 | 110.631 | 112.162 | 110.915 | 0.333 | ns/op | 1.003
>> o.o.b.vm.compiler.IfMinMax.testSingleLong | avgt | 40 | 121.53 | 121.711 | 120.91 | 121.665 | 121.309 | 120.57 | 118.639 | 119.373 | 0.451 | ns/op | 1.002
>> o.o.b.vm.compiler.IfMinMax.testVectorInt | avgt | 40 | 60130.165 | 60062.303 | 61839.776 | 61895.194 | 15887.398 | 15924.502 | 15874.835 | 15667.936 | 101.94 | ns/op | 3.785
>> o.o.b.vm.c...
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   enable more test

Thanks for the updates. Several minor comments remain :-)

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2173:

> 2171:       break;
> 2172:     case BoolTest::ge:
> 2173:       cmov_cmp_fp_ge(op1, op2, dst, src, is_single);

This calls `cmov_cmp_fp_ge` and `cmov_cmp_fp_gt` assembler routines for `BoolTest::ge` and `BoolTest::gt` respectively, but these two assembler routines are NOT implemented in file macroAssembler_riscv.cpp. That seems a bit confusing to me. How about making these two cases unreachable in this function? Or simply implement these two assembler routines?

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1361:

> 1359: //   java code      :  cmp1 > cmp2 ? dst : src
> 1360: //   transformed to :  CMove dst, (cmp1 le cmp2), dst, src
> 1361: // So, cmov_le_fp is invoked instead this method.

I think you mean `cmov_cmp_fp_le` instead of `cmov_le_fp` in this code comment?

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1396:

> 1394: // Clarification:
> 1395: //   java code      :  cmp2 <= cmp1 ? dst : src
> 1396: //   transformed to :  CMove dst, (cmp1 lt cmp2), dst, src

Does this code comment need update? Seems it's the same as the one for `cmov_cmp_fp_lt`.

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

PR Review: https://git.openjdk.org/jdk/pull/24490#pullrequestreview-2759089118
PR Review Comment: https://git.openjdk.org/jdk/pull/24490#discussion_r2038766122
PR Review Comment: https://git.openjdk.org/jdk/pull/24490#discussion_r2038762371
PR Review Comment: https://git.openjdk.org/jdk/pull/24490#discussion_r2038763750