On Mon, 1 Dec 2025 15:13:13 GMT, Hamlin Li <[email protected]> wrote:
>> Hi,
>>
>> This pr add CMoveF/D on riscv, which enable vectorization of statement like:
>> `op_1 bop op_2 ? res_f_d_1 : res_f_d_2 in a loop`.
>>
>> This pr is also a preparation for further vectorization in
>> https://github.com/openjdk/jdk/pull/28231.
>>
>> Previously it's https://github.com/openjdk/jdk/pull/25341, but at that time,
>> C2 SLP has some issue with unsigned comparison, which is now fixed, so it's
>> good to continue the work.
>>
>> # Test
>> ## Jtreg
>>
>> in progress...
>>
>> ## Performance
>>
>> Column names meanings:
>> * p: with patch
>> * p+v: with patch, `-XX:+UseVectorCmov -XX:+UseCMoveUnconditionally` turned
>> on
>> * m: without patch
>> * m+v: without patch, `-XX:+UseVectorCmov -XX:+UseCMoveUnconditionally`
>> turned on
>>
>> #### Average improvement
>>
>> NOTE: With only this PR, it brings performance benefit in case of
>> `CMoveF+CmpF`, `CMoveD+ComD`, `CMoveF+CmpI`, `CMoveD+CmpL`. The data below
>> is based on fullly implmenting the vectorization of
>> `CMoveI/L/F/D+CmpI/L/F/D`, which will be achieved by
>> https://github.com/openjdk/jdk/pull/28231.
>>
>> For details, check the performance data in
>> https://github.com/openjdk/jdk/pull/25341 on riscv.
>> <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;">
>> Opt (m/p) | Opt (m+v/p+v) | Opt (p/p+v) | Opt (m/p+v)
>> -- | -- | -- | --
>> 1.022782609 | 2.198717391 | 2.162673913 | 2.199
>>
>> </google-sheets-html-origin>
>
> Hamlin Li has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - remove log_warning
> - add test cases: BoolTest::ge/gt in enc_cmove_fp_cmp_fp
Nice work! I did not review the RISC-V specific changes, but had a look at the
tests. Wow, there are a lot of them, and that's a good thing :)
I have a few comments below, to consider for improvement.
test/hotspot/jtreg/compiler/c2/irTests/TestConditionalMove.java line 39:
> 37: * @summary Auto-vectorization enhancement to support vector conditional
> move.
> 38: * @library /test/lib /
> 39: * @run driver compiler.c2.irTests.TestConditionalMove
Suggestion:
* @run driver ${test.main.class}
Might as well do that now. Avoids wrong copy of class name, which can lead to
wrong test being run.
test/hotspot/jtreg/compiler/c2/irTests/TestConditionalMove.java line 1564:
> 1562: // @IR(counts = {IRNode.CMOVE_I, ">0", IRNode.CMP_L, ">0"},
> 1563: // applyIf = {"UseVectorCmov", "false"},
> 1564: // applyIfPlatform = {"riscv64", "true"})
Why are these all commented out?
test/hotspot/jtreg/compiler/c2/irTests/TestScalarConditionalMoveCmpObj.java
line 34:
> 32: * @test
> 33: * @summary Test conditional move + compare object.
> 34: * @requires vm.simpleArch == "riscv64"
It would be really nice if we generally have no platform requirements at the
top of the file, but just IR test applyIf instead. That way, everyone benefits
from everyone else's tests and we have less test duplication down the line.
test/hotspot/jtreg/compiler/c2/irTests/TestScalarConditionalMoveCmpObj.java
line 356:
> 354: };
> 355: }
> 356: }
You could use `Generators.java`, it creates "interesting" distributions, and
makes sure we use sufficient special case values.
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/28309#pullrequestreview-3552071451
PR Review Comment: https://git.openjdk.org/jdk/pull/28309#discussion_r2598623788
PR Review Comment: https://git.openjdk.org/jdk/pull/28309#discussion_r2598632136
PR Review Comment: https://git.openjdk.org/jdk/pull/28309#discussion_r2598660768
PR Review Comment: https://git.openjdk.org/jdk/pull/28309#discussion_r2598654446
