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

[Fei Gao]

On Mon, 8 Sep 2025 10:16:16 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> Could you please add a general comment about what this does at the top?

Done

> The name is a bit funny with goo, but that's not your fault. If you have a better name feel free to rename ;)

Renaming `clone_up_backedge_goo` would affect some unrelated files, so I’d prefer to handle the renaming in a separate patch :)

> src/hotspot/share/opto/loopnode.hpp line 1434:
> 
>> 1432:   Node* get_vectorized_drain_input(Node* main_backedge_ctrl, VectorSet& visited,
>> 1433:                                    Node_Stack& clones, Node* main_merge_region,
>> 1434:                                    Node* main_phi);
> 
> We don't just do this for the trip-counter though, right? Because the `main_incr` suggests that a bit here. Could you rephrase to make it more accurate? Do you think that could be worth it? It is also nice to have the analogy to the trip-counter, so I like that in the example ASCII art.

Yes, it applies to all values that increase as the loop iterates. I’m afraid I forgot to rename `main_incr` to a more general name after refactoring the code here. I’ll update it in the next commit. How about renaming it to `main_out`?

> src/hotspot/share/opto/loopopts.cpp line 2466:
> 
>> 2464:       // Find the phi node merging the data from pre-loop and vector main-loop.
>> 2465:       Node_List visit_list;
>> 2466:       Node_List phi_list;
> 
> You are doing this in a loop. And you set no `ResouceMark`. I'm afraid this could end up allocating a lot of memory. What do you think?

The `old_new` map grows within the live ranges of `phi_list` and `visit_list`, so we can’t use `ResourceMark` here. In the new commit, I’ve moved the declarations of these two variables outside the loop and clear them at the start of each new iteration. Does that make sense?

> src/hotspot/share/opto/loopopts.cpp line 2514:
> 
>> 2512:           assert(!has_ctrl(outn) || !has_ctrl(curr) || is_dominator(get_ctrl(curr), get_ctrl(outn)),
>> 2513:                  "Only these nodes controlled by loop exit edge need to be cloned");
>> 2514:           visit_list.push(outn);
> 
> Might we visit nodes more than once? Or is that already prevented?

Yes, we did a check while visiting the node in the list:

        Node* curr = visit_list.at(0);
        visit_list.remove(0);
        Node* newcurr = old_new[curr->_idx];
        if (newcurr != nullptr) { continue; }
        newcurr = curr->clone();
        ...
        old_new.map(curr->_idx, newcurr);

> Do we need to do both fix_data_uses and handle_data_uses_for_vectorized_drain? Ah, they do it one for the old and one for the new loop?
> 
> It is kinda funny that we do a loop here for the old loop, but then do the loop inside fix_data_uses for the other loop - did I understand this right? 

`fix_data_uses` fixes data uses for all nodes in the loop body. The loop here handles nodes collected in `extra_data_nodes`, which may belong to the outer loop or represent special cases.

> We can also do that in a separate RFE first maybe? Because now with the large switch case here things are harder to read and get an overview quickly. What do you think?

Sounds great. :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2510974372
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2510975101
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2510972400
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2503311208
PR Review Comment: https://git.openjdk.org/jdk/pull/22629#discussion_r2510985823