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

[Emanuel Peter]

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

>> src/hotspot/share/opto/vectorization.hpp line 1387:
>> 
>>> 1385: };
>>> 1386: 
>>> 1387: class VTransformGraph : public StackObj {
>> 
>> I was looking for some summary/guidance here but only found it above on L1327 which is hard to find once this is in. How about putting the comments here instead?
>
> I will make a summary here. But I also like having a more broad explanation at the beginning of the `vtransform.hpp` file I'll move all this code to.

I now made a comment at the beginning of the `vtransform.hpp` file, and refer to it from the classes that need more explanation than the 2-3 lines I put there.

>> src/hotspot/share/opto/vectorization.hpp line 1411:
>> 
>>> 1409:   bool _is_trace_align_vector;
>>> 1410:   bool _is_trace_info;
>>> 1411:   bool _is_trace_verbose;
>> 
>> Can these somehow be made `const`?
>
> The tricky thing is with the initialization: I now do that in the constructor body. But if it is `const`, then I need to do that in the initialization list. And I would like to use a local variable `bool is_trace     = _vloop.vtrace().is_trace(TraceAutoVectorizationTag::VTRANSFORM);`. But one can only have local variables after initialization of members. I thought of a few alternative solutions, but all of them are nasty in different ways.
> 
> @chhagedorn Do you have a good idea?

I put it into a separate `VTransformTrace` class, and made the fields `const` there.

>> src/hotspot/share/opto/vectorization.hpp line 1555:
>> 
>>> 1553:   virtual VTransformReductionVectorNode* isa_ReductionVector() { return nullptr; }
>>> 1554: 
>>> 1555:   virtual VTransformApplyStatus apply(const VLoopAnalyzer& vloop_analyzer,
>> 
>> "apply" is quite a generic name. We've discussed about this offline. What about naming it `transform()`? This would match the IGVN naming convention of transforming nodes. Moreover, you already have `find_transformed_input()` which would also support the name `transform()` over `apply()`.
>
> Let's keep discussing this offline then ;)

I think we solved it now, with the `VTransform` class split out of the `VTransformGraph`. This is more encapsulated, and the naming of `VTransoform::apply` makes sense: we apply a transform. `VTransformGraph::apply` was less good, because it is not clear what it means to apply a graph.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1664198783
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1664194590
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1664196859