RFR: 8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph [v2]

[Emanuel Peter]

On Tue, 2 Jul 2024 13:13:45 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/superword.cpp line 1910:
>> 
>>> 1908: // ordered according to the _schedule. This means that all packed memops are consecutive
>>> 1909: // in the memory graph after the reordering.
>>> 1910: void VTransformGraph::apply_memops_reordering_with_schedule() const {
>> 
>> General thought here (was like that before and could also be done separately at some point): Steps (1) - (3) could probably be moved to separate methods.
>
> Could be done in a separate RFE. Also we can mobe the code over to `vectorization.hpp/cpp`. It would at that point even become a class. Would have to experiment to see if that really makes the code easier to read / maintain.

I did the least necessary here to keep the diff small and more reviewable ;)

>> src/hotspot/share/opto/superword.cpp line 3041:
>> 
>>> 3039:   VectorSet vtn_dependencies; // Shared, but cleared for every vtnode.
>>> 3040:   build_edges_for_vector_vtnodes(vtn_dependencies);
>>> 3041:   build_edges_for_scalar_vtnodes(vtn_dependencies);
>> 
>> This is a little confusing since you are not only adding edges but also new vtnodes. Maybe you find a better name. But I suggest to at least add some comments at these methods to mention that. Otherwise, it reads like: Build vector vtnodes, then scalar vtnodes, then connect them somehow (without new nodes being added).
>
> But this is exactly the idea!

I guess the extra `vtnodes` during edge creation is only for inputs that come from outside the loop... hmm.

What about:

build_edges_for_vector_vtnodes -> build_inputs_for_vector_vtnodes
build_edges_for_scalar_vtnodes -> build_inputs_for_scalar_vtnodes

And of course if the inputs are already built, we can just connect those.

>> src/hotspot/share/opto/vectorization.hpp line 1541:
>> 
>>> 1539:   VTransformNode* out(int i) const { return _out.at(i); }
>>> 1540: 
>>> 1541:   bool has_req_or_dep() const {
>> 
>> To match `add_dependency`, I suggest to go with:
>> Suggestion:
>> 
>>   bool has_req_or_dependency() const {
>
> Good idea.

Also renaming
`schedule_collect_nodes_without_req_or_dep -> schedule_collect_nodes_without_req_or_dependency`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1662494937
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1662502350
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1662621568