RFR: 8366702: C2 SuperWord: refactor VTransform vector nodes

[Manuel Hässig]

On Tue, 2 Sep 2025 15:30:06 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 `VTransformElementWiseVectorNode`:
>   - `VTransformReinterpretVectorN...

Thank you for your continued effort on cost modelling, @eme64! I have some minor style comments and questions, but this mostly looks good to me. 

Regarding style, I find the alignment of local variables to be a bit distracting, especially when the aligned "things" are different operations and things are sometimes aligned and sometimes not. However, I do not know the style of the rest of the SuperWord code.

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

> 113:       VTransformBoolVectorNode* vtn_mask_cmp = vtn->in_req(3)->isa_BoolVector();
> 114:       if (vtn_mask_cmp->test()._is_negated) {
> 115:         vtn->swap_req(1, 2); // swap if test was negated.

Suggestion:

      // Inputs must be permuted from (mask, blend1, blend2) -> (blend1, blend2, mask)
      // Or, if the test was negated: (blend1, blend2, mask) -> (blend2, blend1, mask)
      vtn->swap_req(1, 3); // Now, the reqs are negated.
      VTransformBoolVectorNode* vtn_mask_cmp = vtn->in_req(3)->isa_BoolVector();
      if (!vtn_mask_cmp->test()._is_negated) {
        vtn->swap_req(1, 2); // Swap if test was not negated.

This would save to a swap, but I am unsure if this is also more readable.

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();

Suggestion:

  const int sopc = prototype.scalar_opcode();

Nit: whitespace
Or were you trying to align with the line below? Personally, I find this a bit too much, but up to you.

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

> 153:   const VTransformVectorNodePrototype prototype = VTransformVectorNodePrototype::make_from_pack(pack, _vloop_analyzer);
> 154:   const int  sopc = prototype.scalar_opcode();
> 155:   const uint vlen = prototype.vector_length();

As someone that is not familiar with the Superword code: is "pack size" and "vector length" often used interchangeably? if not, then I would keep the name.

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

Changes requested by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/27056#pullrequestreview-3190237084
PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2325655080
PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2325656896
PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2325662867