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

[Emanuel Peter]

On Thu, 18 Apr 2024 09:28:28 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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?
>
> Can you also be consistent with the names all the way through your comments? I suggest you just only use `stride_con`, and not `stride`. You can use `i` and `a`, if you want. But then it would be helpful if you had two lines with identical expressions, but where you make the transition from `i` to `phi`.

Ah. It seems that we require `stride2 / stride` to be a lossless division in the code. A comment about that limitation would be helpful. And I think you should also check if there are tests that cover cases where the division would be lossy.

>> 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?
>
> And why no IR rules for these?

You definately need more tests with IR rules.

>> 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?
>
> Generally, it would be nice if you had more cases where we are checking overflows.

And some with negative strides would be great too.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570364776
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570374047
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570372407