RFR: 8366888: C2: incorrect assertion predicate with short running long counted loop [v4]

[Benoît Maillard]

On Tue, 28 Oct 2025 10:13:32 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> In:
>> 
>> 
>>         for (int i = 100; i < 1100; i++) {
>>             v += floatArray[i - 100];
>>             Objects.checkIndex(i, longRange);
>>         }
>> 
>> 
>> The int counted loop has both an int range check and a long range. The
>> int range check is optimized first. Assertion predicates are inserted
>> above the loop. One predicates checks that:
>> 
>> 
>> init - 100 <u floatArray.length
>> 
>> 
>> The loop is then transformed to enable the optimization of the long
>> range check. The loop is short running, so there's no need to create a
>> loop nest. The counted loop is mostly left as is but, the loop's
>> bounds are changed from:
>> 
>> 
>>         for (int i = 100; i < 1100; i++) {
>> 
>> 
>> to:
>> 
>> 
>>         for (int i = 0; i < 1000; i++) {
>> 
>> 
>> The reason for that the long range check transformation expects the
>> loop to start at 0.
>> 
>> Pre/main/post loops are created. Template Assertion predicates are
>> added above the main loop. The loop is unrolled. Initialized assertion
>> predicates are created. The one created from the condition:
>> 
>> 
>> init - 100 <u floatArray.length
>> 
>> 
>> checks the value of `i` out of the pre loop which is 1. That check fails.
>> 
>> The root cause of the failure is that when bounds of the counted loop
>> are changed, template assertion predicates need to be updated with and
>> adjusted init input.
>> 
>> When the bounds of the loop are known, the assertion predicates can be
>> updated in place. Otherwise, when the loop is speculated to be short
>> running, the assertion predicates are updated when they are cloned.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8366888
>  - whitespaces
>  - review
>  - Merge branch 'master' into JDK-8366888
>  - Update src/hotspot/share/opto/predicates.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/predicates.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopnode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopnode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - whitespaces
>  - fix

Thanks for fixing this @rwestrel. I was not very familiar with assertion predicates before reviewing this, but the logic seems sounds to me now. Nice work

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

> 1194: //     for (int = 0; i < stop - start; i+= stride) { ... }
> 1195: // Template Assertion Predicates added so far were with an init value of start. They need to be updated with the new
> 1196: // init value of 0:

Not being super familiar with assertion predicates, I was a little bit confused at first. I would maybe add something along the lines of:

Suggestion:

// init value of 0. We want the OpaqueLoopInit node on the zero in order to be able to replace it when cloning the predicate.


But feel free to ignore if you think this is obvious.

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

Marked as reviewed by bmaillard (Committer).

PR Review: https://git.openjdk.org/jdk/pull/27250#pullrequestreview-3435756270
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2505007523