RFR: 8366702: C2 SuperWord: refactor VTransform vector nodes [v2]

[Christian Hagedorn]

On Mon, 8 Sep 2025 07:00:54 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> I'm working on cost-modeling, and am integrating some smaller changes from this proof-of-concept PR:
>> https://github.com/openjdk/jdk/pull/20964
>> [See plan overfiew.](https://bugs.openjdk.org/browse/JDK-8340093)
>> 
>> This is a pure refactoring - no change in behaviour. I'm presenting it like this because it will make reviews easier.
>> 
>> ---------------------------------
>> 
>> I have to say: I'm very sorry for this refactoring. I took some decisions in https://github.com/openjdk/jdk/pull/19719 that I'm now partially undoing. I moved too much logic from `SuperWord::output` (now called `SuperWordVTransformBuilder::make_vector_vtnode_for_pack`) to the `VTransform...Node::apply`. https://github.com/openjdk/jdk/pull/19719  was a roughly 1.5k line change, and I took about a 0.3k misstep that I'm now correcting here ;)
>> 
>> I had accidentially made the `VTransformGraph` too close to the `PackSet`, and not close enough to the future vectorized C2 Graph. And that makes some future changes hard.
>> 
>> My vision:
>> - VLoop / VLoopAnalyzer look at the scalar loop and prepare it for SuperWord
>> - SuperWord creates the `PackSet`: some nodes are packed, all others are scalar.
>> - `SuperWordVTransformBuilder` converts the `PackSet` into the `VTransformGraph`
>> - The `VTransformGraph` very closely represents the C2 vectorized loop after vectorization
>>   - It does not need to know which `nodes` it packs, it rather just needs to know how to generate the new vector nodes
>>   - That means it is straight-forward to compute cost
>>   - And it also makes optimizations on that graph easier
>>   - And the `apply` methods are simpler too
>> 
>> ----------------------------------
>> 
>> So therefore, the main goal was to make the `VTransform...Node::apply` calls simpler again. And move the logic back to `SuperWordVTransformBuilder::make_vector_vtnode_for_pack`.
>> 
>> One important step to making the the `VTransformGraph` less of a `PackSet` is to remove reliance on `nodes` for the vector nodes.
>> 
>> What I did:
>> - Moving a lot of the logic in `VTransformElementWiseVectorNode::apply` to `SuperWordVTransformBuilder::make_vector_vtnode_for_pack`.
>>   - Will make it easier to optimize and compute cost in future RFE's.
>> - `VTransformVectorNodePrototype`: packs a lot of the info for `VTransformVectorNode`.
>>   - pass info about `bt`, `vlen`, `sopc` instead of the `pack` -> allows us to eventually remove the dependency on `nodes`.
>> - New vector nodes, they are special cases I split away from ...
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review comment implemented

Nice refactoring! Just some small suggestions, otherwise, it looks good to me.

src/hotspot/share/opto/superwordVTransformBuilder.cpp line 154:

> 152:   Node* p0 = pack->at(0);
> 153:   const VTransformVectorNodePrototype prototype = VTransformVectorNodePrototype::make_from_pack(pack, _vloop_analyzer);
> 154:   const int sopc     = prototype.scalar_opcode();

You use this at other places already but could be more readable when renamed to `scalar_opc` or `scalar_opcode`.

src/hotspot/share/opto/superwordVTransformBuilder.cpp line 173:

> 171:     vtn = new (_vtransform.arena()) VTransformBoolVectorNode(_vtransform, prototype, kind);
> 172:   } else if (p0->is_CMove()) {
> 173:     vtn = new (_vtransform.arena()) VTransformElementWiseVectorNode(_vtransform, p0->req(), prototype, Op_VectorBlend);

You also seem to use `p0->req()` a lot. Should we create a `const` above for easier access? Could we also have a better name than `p0`? But again, you are using `p0` a lot at other places already and it might be evidently clear in this context.

src/hotspot/share/opto/vtransform.hpp line 600:

> 598: 
> 599: // Bundle the information needed for vector nodes.
> 600: class VTransformVectorNodePrototype : public StackObj {

Prototype sounds like actually having something concrete, not fully set up or just something to copy/clone from as a starting point. But IIUC, this class just serves as a holder class for some information. How about naming it `Prototype` -> `Properties`?

src/hotspot/share/opto/vtransform.hpp line 617:

> 615: 
> 616: public:
> 617:   static VTransformVectorNodePrototype make_from_pack(const Node_List* pack, const VLoopAnalyzer& vloop_analyzer) {

When switching to "Properties", you could also rename this to something like "fetch_from_pack" since `make` also suggests to actually creating a dummy-kind node when it's only trying to fetch useful information.

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

PR Review: https://git.openjdk.org/jdk/pull/27056#pullrequestreview-3195283374
PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2329386959
PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2329409899
PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2329374048
PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2329392637