RFR: 8366427: C2 SuperWord: refactor VTransform scalar nodes

[Emanuel Peter]

On Fri, 29 Aug 2025 14:31:08 GMT, Vladimir Kozlov <kvn 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
>> 
>> This is a pure refactoring - no change in behaviour. I'm presenting it like this because it will make reviews easier.
>> 
>> The goal is to split up some cases that are currently treated the same, but will alter have different behavior. There may be a little bit of code duplication, but the code will soon be made different ;)
>> 
>> We split the `VTransformScalarNode`:
>> - `VTransformMemopScalarNode`
>>   - Uses that only wanted scalar mem nodes can now directly check for `isa_MemopScalar`.
>>   - We can directly store the `_vpointer` in a field, that way we don't need to do a lookup via `vloop_analyzer`. This could also be helpful later on if we ever do widening (unrolling during auto vectorization): we could then do the necessary modifications to the `vpointer`.
>> - `VTransformLoopPhiNode`
>>   - Later on, they will play a more special role, they will give us easy access to the beginning state of the loop body and the backedges.
>> - `VTransformCFGNode`
>>   - Calling them scalar nodes is not 100% accurate. We'll probably have to further refine them later on. But splitting them off now seems like a reasonable choice. Once we do if-conversion we'll have to do more work on CFG.
>> - `VTransformDataScalarNode`
>>   - These represent all the normal "calculation" nodes in the loop.
>> - `VTransformInputScalarNode` -> `VTransformOuterNode`:
>>   - For now, we are still just tracking input nodes, but soon we will need to track input and output nodes: basically just the 1-hop neighbourhood of nodes outside the loop. I'm already renaming them now, so it will be less noise later.
>> 
>> I decided to rather split up more, and avoid the `VTransformScalarNode` together, avoiding having to override overrides - that can be really confusing (e.g. what I had with `is_load_in_loop`).
>
> src/hotspot/share/opto/vtransform.hpp line 457:
> 
>> 455: class VTransformMemopScalarNode : public VTransformNode {
>> 456: private:
>> 457:   MemNode* _node;
> 
> Why not have `_node` in `VTransformNode` class and use `MemNode* node() const { return _node->as_Mem(); }` in this class?  similar to other new classes.

I feared this might get a question ;)

I'd like to do it this way, and later there will need to be more changes. There will also be changes for `_nodes` in the vector nodes.

Below some more thoughts - reading optional ;)

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

`VTransformNode` is too high up - it is the superclass of all. And not all have a `node`. The vector nodes have a list of nodes.

One option would be re-introducing some `VTransformScalarNode` that does nothing but hold that shared `_node` field. But I'd like to avoid having a public accessor for all of the subclasses. But picking a good name that unites all the subclasses is not so easy (data, memory, CFG, ... ). Conceptually, having such an in-between-class is not very helpful I fear.

In the long-run, we will probably not just have the "identity transform" with `_node`, but these 3:
- identity transform: reference a `node`, and keep it (maybe rewire inputs if memory is reordered).
- add new node, where there is no old reference (e.g. `VTransformConvI2LNode`, there will be more like extract).
- copy node: if we ever do "virtual unrolling" / widening, some nodes may not be vectorized (widened) and instead need to be duplicated/copied.

So maybe eventually I'll need more than just a `_node`:
- `_node`: for identity transform or copy
- an opcode to generate a new node
- an enum that says if we do: identity vs copy vs generate

We might need to a similar thing for vector nodes.

It is a little hard to model everything perfectly at the current state, without introducing massive code changes.
I'd rather "atomize" everything and duplicate some code now. Later, it will be easier to unite things again.

It is possible that I'll end up with something that covers everything, and put it in `VTransformNode`:
- something like `VTransformNodePrototype` in the POC PR https://github.com/openjdk/jdk/pull/20964.
  - `_node`: a reference for identity transform, copy. But even if we create a new node, we might want to have an "approximate origin" to copy the node notes from.
  - enum that says if we do identity vs copy vs generate
  - `vlen`: 1 for scalar, >1 for vector
  - `element_type` (BasicType)
  - `opcode`: the target opcode (scalar opcode when scalar, vector if to be vectorized)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27002#discussion_r2310448015