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

[Christian Hagedorn]

On Wed, 3 Jul 2024 13:33:23 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> I'll see if I can do this directly with the `new` operator...
>
> I think having a `_arena` field in the `SuperWordVTransformBuilder` would be confusing and not a good idea. People might think that this is an arena that is used during the life-cycle of the builder. But this `_graph.arena()`, now `_vtransform.arena()` is used for the life-cycle of the `vtransform`, which outlives the builder. We could have a `_vtransform_arena` field, but this is not really better than `_vtransform.arena()` IMHO.

I agree that "arena" could be confused for an arena to be used for all kinds of stuff during the life-cycle for the class while it is the vtransform-arena. `_vtransform.arena()` is perfectly fine. I would have preferred a field since you access it a lot but I guess it's just personal taste ;-)

>> 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.

Good solution!

>> 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.

Good idea!

>> 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.

Yes, I think we found a good solution here :-) "transform.apply()" is now clear.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1665429341
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1665429504
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1665429731
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1665429612