RFR: 8325589: C2 SuperWord refactoring: create VLoopAnalyzer with Submodules

[Emanuel Peter]

On Sat, 10 Feb 2024 17:52:46 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> Subtask of https://github.com/openjdk/jdk/pull/16620
> 
> SuperWord is a nasty large class, and it is difficult to know what impacts what.
> Plus, in the future, we may want to reuse some components of SuperWord in other Vectorizers.
> Hence, I pull some parts out of SuperWord, and put it in a new class `VLoopAnalyzer`.
> This has submodules:
> 
> 
> VLoopAnalyzer
>   VLoopReductions
>   VLoopMemorySlices
>   VLoopBody
>   VLoopTypes
> 
> 
> **Future Work**
> In my draft for https://github.com/openjdk/jdk/pull/16620, I also created a submodule `VLoopDependenceGraph`.
> But that is also a lot of code change.
> I decided to do this in a separate RFE, to keep this patch here reasonably short.
> 
> I also left many functions in `superword.cpp`, even though their classes are in `vectorization.hpp`.
> I think it is easier to review them in their old palces, with minor changes, for now.
> In a future RFE, I can then cleanly copy them, without any changes to them.

src/hotspot/share/opto/superword.cpp line 410:

> 408: #endif
> 409:     return false;
> 410:   }

Note: moved to `VLoopAnalyzer::setup_submodules_helper`

src/hotspot/share/opto/superword.cpp line 494:

> 492:     return false;
> 493:   }
> 494: 

Note: moved to `VLoopAnalyzer::setup_submodules_helper`

src/hotspot/share/opto/superword.cpp line 506:

> 504:   // Compute vector element types
> 505:   compute_vector_element_type();
> 506: 

Note: moved to `VLoopAnalyzer::setup_submodules_helper`

src/hotspot/share/opto/superword.cpp line 767:

> 765: 
> 766:   ResourceMark rm;
> 767:   GrowableArray<Node*> slice_nodes;

Note: replaces `SuperWord::_nlist`

src/hotspot/share/opto/superword.cpp line 1100:

> 1098:       if (t1 == s2) {
> 1099:         // both nodes are reductions and connected
> 1100:         return true;

Note: refactored it a bit: basically just removed the `depth` part, shifted the rest down.
This is not a necessary check (after all we still check that s1 def and s2 is use).
It was probably just a speed optimization, to avoid doing traversal if the depth is known.
But I would like to separate the `depth` info (part of the dependence graph) from the reductions logic.

src/hotspot/share/opto/superword.cpp line 1170:

> 1168:   assert(bsize != 0, "valid size");
> 1169:   return bsize;
> 1170: }

Note: moved to `VLoopTypes`

src/hotspot/share/opto/superword.cpp line 1891:

> 1889:       // Unmark reduction if no parent pack or if not enough work
> 1890:       // to cover reduction expansion overhead
> 1891:       _loop_reductions.remove(p0->_idx);

Note: "unmarking" reductions is not necessary, I think it has no effect either way. We will never pack this reduction any more anyway.
I want to remove this, so that the reductions logic in `VLoopReduction` can be read-only in SuperWord. This is much cleaner. It would also allow us to reuse the `VLoopReduction` data for multiple vectorizer strategies, if we want to do that in the future.

src/hotspot/share/opto/superword.cpp line 2668:

> 2666: #endif
> 2667: 
> 2668:       _block.at_put(i, vn);

Note: this is useless.
At this point we have committed to vectorizing, there is no way out now.
And replacing all nodes of the pack in the block with one vector node does also not make much sense, and it simply does not do anything.

Plus, it would be nice to have the `VLoopBody` as read-only in SuperWord.

src/hotspot/share/opto/superword.cpp line 3147:

> 3145: // Normally the type of the add is integer, but for packed character
> 3146: // operations the type of the add needs to be char.
> 3147: void SuperWord::compute_vector_element_type() {

Note: I improved the description and put it at the head of `VLoopTypes`.

src/hotspot/share/opto/superword.cpp line 3305:

> 3303:   }
> 3304:   return vt1 == vt2;
> 3305: }

Note: moved it to `VLoopTypes`

src/hotspot/share/opto/superword.hpp line 260:

> 258:     return TraceSuperWord ||
> 259:            vloop().vtrace().is_trace(TraceAutoVectorizationTag::SW_MEMORY_SLICES);
> 260:   }

Note: replaced with `vloop().is_trace_memory_slices()` since all the code is now in `VLoopMemorySlices`.
Tag is renamed to `MEMORY_SLICES`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485319461
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485319575
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485319674
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485319991
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485321553
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485321762
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485322665
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485323446
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485324658
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485325136
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1485326358