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

[Emanuel Peter]

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

>> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   update comments to clarify on type casting
>
> 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`.

> 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?

> 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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570358316
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570335960
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1570371373