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

[Emanuel Peter]

On Tue, 2 Jul 2024 10:24:27 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> src/hotspot/share/opto/superword.cpp line 3181:
>> 
>>> 3179: }
>>> 3180: 
>>> 3181: VTransformNode* SuperWordVTransformBuilder::get_vtnode_vector_input_at_index(const Node_List* pack, const int index) {
>> 
>> Note: analogue from old `SuperWord::vector_opd`.
>
> "get" suggests that we only get something that already exists but we are also creating new nodes. Can you add a comment here for clarification what the method is about?

Adding comment and renaming:
`get_vtnode_vector_input_at_index -> get_or_make_vtnode_vector_input_at_index`

>> src/hotspot/share/opto/superword.cpp line 3262:
>> 
>>> 3260:   vtn = new (_graph.arena()) VTransformInputScalarNode(_graph, n);
>>> 3261:   set_vtnode(n, vtn);
>>> 3262:   return vtn;
>> 
>> Note: Since we want the loop-internal nodes to be able to reference all inputs as `vtnodes`, we must pack the inputs that are outside the loop also into special `VTransformInputScalarNode`s.
>
> Can you add this explanation also to the `VTransformInputScalarNode` class?

Good idea, done.

>> src/hotspot/share/opto/vectorization.hpp line 1449:
>> 
>>> 1447: 
>>> 1448:   bool schedule();
>>> 1449:   void apply();
>> 
>> Note: these are the public methods. In the future, we can add more, like `cost()` from the cost-model, or `optimize()` if we have optimizations, or maybe `if_convert()`.
>
> "graph.apply()" seems quite generic. Could you find a more specific name? Maybe `transform_vtnodes()`?

I think `VTransformGraph::apply` is much better than `VTransformGraph::transform_vtnodes`. Because I want to say that we are "applying" or "executing" the transform, which operates on the C2 graph. We are thus transforming some C2 nodes into some other C2 nodes. `transform_vtnodes` suggests that we are transforming the vtnodes, but "from what into what?" would be my intuitive question if I read this.

Would `apply_to_C2_nodes` or `apply_to_C2_graph` be better for you? Or do you have any other ideas?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1662536105
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1662558238
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1662613075