On Thu, 10 Apr 2025 12:08:29 GMT, Hamlin Li <m...@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
