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

[Fei Gao]

On Wed, 14 Jan 2026 14:22:58 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/loopTransform.cpp line 1413:
>> 
>>> 1411:   // we now need to make the fall-in values to the vectorized drain
>>> 1412:   // loop come from phis merging exit values from the pre loop and
>>> 1413:   // the main loop.
>> 
>> Suggestion:
>> 
>>   // the main loop, see "drain_input".
>
> Would this be correct? It would allow the reader to search for "drain_input" and immediately find the right point to focus in the ASCII art below :)

Yes, that's correct. That looks better. I'll update it.

>> src/hotspot/share/opto/loopTransform.cpp line 1460:
>> 
>>> 1458:   // TestVectorizedDrainLoop.java.
>>> 1459:   Node* drain_input = nullptr;
>>> 1460:   Node* iv_after_main = main_phi->in(LoopNode::LoopBackControl);
>> 
>> Is the `phi` we are looking at here always only the `iv`/trip counter?
>> - If always `iv`: how do we patch the other `phi`s? And is this loop not covering all phis? https://github.com/openjdk/jdk/pull/22629/files#diff-6a59f91cb710d682247df87c75faf602f0ff9f87e2855ead1b80719704fbedffL1770-L1781
>> - If we cover all phis: we should probably change the naming of the variables to indicate that it is any `phi` and not just the `iv`.
>> 
>> What do you think?
>
> Also: how sure are you that the backedge `main_phi->in(LoopNode::LoopBackControl)` is the same as the the value after main `iv_after_main`?
> What if we did some split-through-phi action at some point? Example:
> 
> x = ...
> LOOP:
>     x = op(x);
>     // x now serves as exit value and backedge value
>     exit check;
>     goto LOOP;
> 
> If we split `op` through the LOOP Phi, we get:
> 
> x = ...
> x = op(x);
> LOOP:
>     // the exit value is the phi
>     exit check;
>     x = op(x);
>     // x after op is the backedge
>     goto LOOP;
> 
> 
> I'm not sure this currently ever happens, but what if it did?

> * If we cover all phis: we should probably change the naming of the variables to indicate that it is any `phi` and not just the `iv`.
> 
> What do you think?

Yes, we’re looking at all variables that change as the loop iterates, not just the trip counter. How about renaming it to `main_exit_value` or `value_after_main_loop`?

> So maybe it should not be: `Node* iv_after_main = main_phi->in(LoopNode::LoopBackControl);` But instead: `Node* main_backedge = main_phi->in(LoopNode::LoopBackControl);` Because it is at that point not yet clear that it is really the `after_main` value, of if we need to clone it first.

Yeah, that makes sense. We probably do need two separate variables here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2694674918
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2694752041
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2694968788