RFR: 8307084: C2: Vectorized drain loop is not executed for some small trip counts [v4]

[Emanuel Peter]

On Tue, 20 Jan 2026 11:12:35 GMT, Fei Gao <fgao at openjdk.org> wrote:

>> src/hotspot/share/opto/loopTransform.cpp line 1841:
>> 
>>> 1839:   }
>>> 1840: }
>>> 1841: 
>> 
>> A few questions:
>> - Why not cache the `skip_assertion_predicates_with_halt` values? Do the values change over time? If there are lots of predicates, you will do a traversal over and over again.
>> - Why do we need this special logic for the `drain` loop cloning? What makes it different to other cloning cases?
>> - The new ctrl you set is either at the post-head entry, or after skipping the predicates. Why did you chose those?
>
> Thanks for the questions. Great points.
> 
>> Why not cache the `skip_assertion_predicates_with_halt` values? Do the values change over time? If there are lots of predicates, you will do a traversal over and over again.
> 
> You’re right. There’s no strong reason not to cache the result of `skip_assertion_predicates_with_halt() `here. We can do that to avoid repeated traversals when there are many predicates. I’ll refine the code accordingly.
> 
>> Why do we need this special logic for the drain loop cloning? What makes it different to other cloning cases?
> 
> Drain-loop cloning is significantly more complex than other cloning cases.
> 
> When cloning a `post loop`, the loop structure is relatively simple and does not involve a `pre-loop` or a `minimum-trip guard`. The control-flow shape typically looks like:
> 
>                 ...
>            /       \
>         ...     multiple predicates
>          /           \
>                     EntryControl
>       /                 \
>                       main loop head
> 
> In this case, some nodes in the main loop may take any control node between the predicate `IfTrue` nodes and the loop `EntryControl` as their control input, meaning they can float along that control chain. When such nodes are cloned, their control inputs need to be fixed to the corresponding nodes in the cloned loop.
> 
> This is already handled by `initialize_assertion_predicates_for_post_loop()`, which correctly fixes control inputs for `TemplateAssertionPredicates`. See https://github.com/openjdk/jdk/blob/c5f288e2ae2ebe6ee4a0d39d91348f746bd0e353/src/hotspot/share/opto/predicates.cpp#L157
> 
> However, when inserting a `drain loop`, the control-flow structure is more involved:
> 
>          main zero-trip guard
>              /      \
>         IfFalse    IfTrue
>            /          \
>         ...       multiple predicates
>          /              \
>                         EntryControl
>       /                    \
>                       main loop head
> 
> 
> Here, the main loop is preceded by both a `pre-loop` and a `minimum-trip guard`. As a result, control inputs of nodes inside the main loop may originate anywhere from the `IfTrue` of the `minimum-trip guard` down to the loop `EntryControl`. These cases are outside the scope of `initialize_assertion_predicates_for_post_loop()`, which only handles `TemplateAssertionPredicates`.
> 
> In practice, I’ve also observed nodes attached to `InitializedAssertionPredicates`, which are not covered by that helper either. 
> 
> This is why we need separate, more ...

Thanks for the explanations! They sound reasonable to me. Though eventually it would be good if @chhagedorn or @rwestrel looked at this, they are more familiar with this code.

One more question here: could it be that one node that you now conservatively pin further down actually already has a use in a predicate further up, and now we'd create a `bad graph` cycle?

> The worklist should be empty before we reach this point.

Then we should add an assert, both for correctness and the benefit of the reader :)

>> src/hotspot/share/opto/loopopts.cpp line 2485:
>> 
>>> 2483:   }
>>> 2484: 
>>> 2485:   Node_List visit_list;
>> 
>> Suggestion:
>> 
>>   ResourceMark rm;
>>   Node_List visit_list;
>> 
>> Can we do this, or do we run into issues?
>
> No, we can’t. I think we discussed this before. Since `old_new` can grow within this scope, we can’t use `ResourceMark` here.

Oh dear, we probably discussed this before. Ok, that's a shame, but fine. If it should create issues down the road we can see what to do about then.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2708426565
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2708439901
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2708436382