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

[Christian Hagedorn]

On Fri, 12 Sep 2025 08:57:57 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.

Sorry for letting you wait - I had this on my TODO list for quite some time. The fix looks good to me! I only have some small comments.

Thanks for the attribution and nice that you were able to extract another test that shows the root problem directly!

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

> 1163:   ClonePredicateToTargetLoop _clone_predicate_to_loop;
> 1164:   PhaseIdealLoop* const _phase;
> 1165:   Node* _new_init;

Can be made `const`:

Suggestion:

  Node* const _new_init;

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

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

Some small suggestions here. I suggest to also add some visualization to quickly see what we do here.
Suggestion:

// For an int counted loop, try_make_short_running_loop() transforms the loop from:
//     for (int = start; i < stop; i+= stride) { ... }
// to
//     for (int = 0; i < stop - start; i+= stride) { ... }
// Template Assertion Predicates added so far were with an init value of start. They need to be updated with the new 
// init value of 0:
//                                zero      
//        init                     |       
//         |           ===>   OpaqueLoopInit   init
//  OpaqueLoopInit                         \   /
//                                          AddI
//

src/hotspot/share/opto/node.hpp line 2168:

> 2166: 
> 2167:   // Defines an action that should be taken when we visit a target node in the BFS traversal.
> 2168:   virtual void target_node_action(Node* node, uint i) = 0;

Maybe you can name `node` `child` and add the following comment to better describe it:

Suggestion:

// Defines an action that should be taken when we visit a target node in the BFS traversal.
// To give more freedom, we pass the direct child node to the target node such that
// child->in(i) == target node. This allows to also directly replace the target node instead
// of only updating its inputs.
  virtual void target_node_action(Node* child, uint i) = 0;

src/hotspot/share/opto/predicates.cpp line 206:

> 204: }
> 205: 
> 206: // Clone this Template Assertion Predicate and use the expression passed as argument as init.

Suggestion:

// Clone this Template Assertion Predicate and replace the old OpaqueLoopInit node with 'new_init'.
// Note: 'new_init' could also have the 'OpaqueLoopInit` as parent node further up.

src/hotspot/share/opto/predicates.cpp line 249:

> 247:   }
> 248: 
> 249:   void target_node_action(Node* node, uint i) override {

I also suggest to add an assert here just to be sure:

assert(node->in(i)->is_OpaqueLoopStride(), "must be OpaqueLoopStride");

src/hotspot/share/opto/predicates.cpp line 279:

> 277:   }
> 278: 
> 279:   void target_node_action(Node* node, uint i) override {

Should hold but might still not hurt to add the following assertion:

assert(node->in(i)->is_OpaqueLoopInit(), "must be old OpaqueLoopInit);

src/hotspot/share/opto/predicates.cpp line 1144:

> 1142: 
> 1143: // Clones the provided Template Assertion Predicate to the head of the current predicate chain at the target loop and
> 1144: // replace current OpaqueLoopInit with the expression passed as argument.

Suggestion:

// replaces the current OpaqueLoopInit with 'new_init'.
//  Note: 'new_init' could also have the 'OpaqueLoopInit` as parent node further up.

test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate2.java line 1:

> 1: /*

Could the two tests also be merged?

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27250#pullrequestreview-3323632416
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2420490429
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2420281093
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2420413917
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2420584197
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2420591008
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2420424787
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2420496983
PR Review Comment: https://git.openjdk.org/jdk/pull/27250#discussion_r2420602420