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

[Emanuel Peter]

On Thu, 15 Jan 2026 16:06:21 GMT, Fei Gao <fgao at openjdk.org> wrote:

>>> * 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`?
>
>> 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?
> 
> Great question—and yes, that’s exactly why this logic is a bit more involved.
> 
> If I understand correctly, in the second scenario—after a split-through-phi—the value we need after the main loop is not the original x directly. Instead, we also need to apply an additional op(x) so that the resulting value can serve as the input to the drain loop.
> 
> 
> x = ...
> y = op(x);
> MAIN_LOOP:
>     // the exit value is the phi
>     exit check;
>     y = op(x);
>     // x after op is the backedge
>     goto MAIN_LOOP;
> 
> MAIN_EXIT:
>     y = op(x);  // the newly created op(x) outside the main-loop body
> 
> DRAIN_LOOP:
>     // the exit value is the phi
>     exit check;
>     y = op(x);
>     // x after op is the backedge
>     goto DRAIN_LOOP;
> 
> 
> 
> Let's walk through the code step by step.
> 
> 
>   Node* drain_input = nullptr;
>   Node* iv_after_main = main_phi->in(LoopNode::LoopBackControl);
> 
> At this point, we do **not** assume that `iv_after_main` is already the correct “after-main” value.
> 
>   if (get_ctrl(iv_after_main) != main_backedge_ctrl) {
>     drain_input = find_merge_phi_for_vectorized_drain(iv_after_main, main_merge_region);
>   }
> 
> We first check whether `main_phi->in(LoopNode::LoopBackControl)` is pinned in the backedge block.
> 
> 1. If `get_ctrl(iv_after_main) != main_backedge_ctrl`, then `main_phi->in(LoopNode::LoopBackControl)` is **not** in the backedge block. It should be **either in the main loop body or in the main exit block.**
> 
> - If it is still in the main loop body, we won’t be able to find a valid merge phi via its uses, so **`drain_input` remains nullptr**.
> 
> - If it is already in the main exit block, we can find an existing valid merge phi via its uses—this corresponds to the first scenario you described, **`drain_input` won't be nullptr**.
> 
> 2. If `get_ctrl(iv_after_main) == main_backedge_ctrl`, then the `main_phi->in(LoopNode::LoopBackControl)` is pinned in the b...

Ah, I see, so the magic is supposed to happen in `clone_up_backedge_goo`. Thanks for all the detailed explanations! I think I had missed the recursive approach.

I had just relied on the old comment above `clone_up_backedge_goo` that only talks about `n` and its direct clone. It fails to mention any of the recursive part, that walks the whole chain.

I think it would be worth spelling it out a bit more for `clone_up_backedge_goo`, and writing down why what it does is correct. I think that would help a lot, especially because you are now using it in a new way, and so we have to make sure things are correct, and it is easy for the reader to see why :)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2697596988