RFR: 8328528: C2 should optimize long-typed parallel iv in an int counted loop [v3]

[Emanuel Peter]

On Wed, 17 Apr 2024 15:37:30 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

>> Currently, parallel iv optimization only happens in an int counted loop with int-typed parallel iv's. This PR adds support for long-typed iv to be optimized. 
>> 
>> Additionally, this ticket contributes to the resolution of [JDK-8275913](https://bugs.openjdk.org/browse/JDK-8275913). Meanwhile, I'm working on adding support for parallel IV replacement for long counted loops which will depend on this PR.
>
> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update comments to clarify on type casting

Nice work! I like it, but you need to improve the comments and tests.

src/hotspot/share/opto/loopnode.cpp line 3921:

> 3919: //    int a = init2
> 3920: //    for (int i = init; i < limit; i += stride) {
> 3921: //      a = init2 + (i - init) * (stride2 / stride)

I like that you are putting comments here, I think it will help. But they seem not quite correct.
If `stride2 = 2` and `stride = 4`, then the division would be rounded down to zero.
Can you be more precise about the order of operations, and rounding issues?

test/hotspot/jtreg/compiler/c2/irTests/TestCountedLoopIV.java line 24:

> 22:  */
> 23: 
> 24: package compiler.c2.irTests;

Putting IR tests into the `irTests` directory is what we did at the beginning, when we assumed IR tests would not be widely adopted. But now it makes more sense to put this test where it belongs "thematically". I suggest you put it under `compiler/loopopts`, or even in a new subdirectory: `compiler/loopopts/parallel_iv`.

Also the name of this test could be more expressive: `TestLongParallelIvInIntCountedLoop.java`

test/hotspot/jtreg/compiler/c2/irTests/TestCountedLoopIV.java line 60:

> 58:     }
> 59: 
> 60:     private static int testIntCountedLoopWithIntIVZero(int stop) {

Why do you not have a `@Test` for every test? Are you sure that these will even be compiled currently?

test/hotspot/jtreg/compiler/c2/irTests/TestCountedLoopIV.java line 63:

> 61:         int a = 0;
> 62:         for (int i = 0; i < stop; i++) {
> 63:             a += 0; // we unfortunately have to repeat ourselves because the operand has to be a constant

I don't understand your comment. Why is this test interesting?

test/hotspot/jtreg/compiler/c2/irTests/TestCountedLoopIV.java line 102:

> 100:         long a = 0;
> 101:         for (int i = 0; i < stop; i++) {
> 102:             a += Long.MAX_VALUE;

Can you also try a value that is just slightly over int_max and one slightly below int_min?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18489#pullrequestreview-2008308135
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570354038
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570329596
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570335355
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570337160
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570339249