RFR: 8366702: C2 SuperWord: refactor VTransform vector nodes

[Emanuel Peter]

On Fri, 5 Sep 2025 17:20:36 GMT, Manuel Hässig <mhaessig 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 ...
>
> 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.

It would be a nice optimization if swap was expensive. But it is not really. I think I prefer the more readable solution  here. But it's a bit of a toss-up. If another reviewer has a preference I'm willing to go with the majority ;)

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

Yes, I was trying to get alignment. I'll try some alternatives.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2329219093
PR Review Comment: https://git.openjdk.org/jdk/pull/27056#discussion_r2329220235