RFR: 8332442: C2: refactor Mod cases in Compile::final_graph_reshaping_main_switch() [v5]

[Kangcheng Xu]

On Wed, 11 Sep 2024 12:00:46 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   allow one div on platforms without hardware divmod
>
> Generally the clean-up idea is good! A few comments.

Updated PR per @chhagedorn's requests. Waiting for GHA to complete.

> test/hotspot/jtreg/compiler/c2/TestDivModNodes.java line 36:
> 
>> 34:  * @library /test/lib /
>> 35:  * @run main/othervm -XX:+UseDivMod compiler.c2.TestDivModNodes
>> 36:  * @run main/othervm -XX:-UseDivMod compiler.c2.TestDivModNodes
> 
> You should not pass the flags like that to the IR framework tests. It will not take these flags into account. What you should do instead is using the IR framework provided methods:
> 
> 
>  * @run driver compiler.c2.TestDivModNodes
> ...
>     public static void main(String[] args) {
>         TestFramework.runWithFlags("-XX:-UseDivMod");
>         TestFramework.runWithFlags("-XX:+UseDivMod");
>     }
> 
> Note that `UseDivMod` is a C2 specific flag. So, you should also add a `@requires vm.compiler2.enabled`.

Thanks for pointing out! This is very critical. I've updated tests with `runWithFlags()`

> test/hotspot/jtreg/compiler/c2/TestDivModNodes.java line 58:
> 
>> 56: 
>> 57:         verifyResult(dividend, divisor, q, r, TestDivModNodes::signedIntDiv, TestDivModNodes::signedIntMod);
>> 58:     }
> 
> Couldn't you just make `q` and `r` fields and then check them from within a `@Run` method?
> 
> Something like that (not tested):
> 
> static final Random RANDOM = AbstractInfo.getRandom();
> int divResult;
> int modResult;
> ...
> @Test
> @IR(...)
> void testSignedIntDivMod(int dividend, int divisor) {
>     divResult = dividend / divisor;
>     modResult = dividend % divisor;
> }
> 
> @Run(test="testSignedIntDivMod")
> void run() {
>     int q = RANDOM.nextInt();
>     int r = RANDOM.nextInt();
>     testSignedIntDivMod(q, r);
>     Asserts.assertEQ(q / r, divResult);
>     Asserts.assertEQ(q % r, modResult);
> }
> 
> Since the IR framework is single threaded, it should not be a problem to pass the result like that over fields.

Good point. I was aiming for generated IR to be as simple as possible. This is even better. I adopted your suggestions, thanks!

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

PR Comment: https://git.openjdk.org/jdk/pull/20877#issuecomment-2344054965
PR Review Comment: https://git.openjdk.org/jdk/pull/20877#discussion_r1755040226
PR Review Comment: https://git.openjdk.org/jdk/pull/20877#discussion_r1755045657