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

[Emanuel Peter]

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

>> Fei Gao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits:
>> 
>>  - Fix build failure after rebasing and address review comments
>>  - Merge branch 'master' into optimize-atomic-post
>>  - Fixed new test failures after rebasing and refined parts of the code to address review comments
>>  - Merge branch 'master' into optimize-atomic-post
>>  - Merge branch 'master' into optimize-atomic-post
>>  - Clean up comments for consistency and add spacing for readability
>>  - Fix some corner case failures and refined part of code
>>  - Merge branch 'master' into optimize-atomic-post
>>  - Refine ascii art, rename some variables and resolve conflicts
>>  - Merge branch 'master' into optimize-atomic-post
>>  - ... and 3 more: https://git.openjdk.org/jdk/compare/a8552243...ab1de504
>
> 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 :)

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

> src/hotspot/share/opto/loopTransform.cpp line 1473:
> 
>> 1471:     // otherwise return 'iv_after_main'.
>> 1472:     iv_after_main = clone_up_backedge_goo(main_backedge_ctrl, main_merge_region->in(2),
>> 1473:                                           iv_after_main, visited, clones);
> 
> I think this part would make more sense if we actually started with a variable `main_backedge` instead of `after_main`, and then the clone gets us the `after_main` value, because now it has been cloned out of the 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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2690652154
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2690744427
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2690779855