RFR: 8312332: C2: Refactor SWPointer out from SuperWord

[Emanuel Peter]

On Tue, 25 Jul 2023 08:51:38 GMT, Pengfei Li <pli at openjdk.org> wrote:

> As discussed in JDK-8308994, we should first do some refactoring work before proceeding with the new post loop vectorization. In this patch, we have done the following.
> 
> 1) We have created new C2 source files `vectorization.[cpp|hpp]` for shared logics and utilities for C2's auto-vectorization. So far we have moved class `SWPointer` and `VectorElementSizeStats` here from `superword.[cpp|hpp]`.
> 
> 2) We have decoupled `SWPointer` from class `SuperWord` and renamed it to `VPointer` as it will be used by vectorizers other than SuperWord. The original class `SWPointer` and its inner class `Tracer` both have a `_slp` field initialized in their constructors. In this patch, we have replaced them by other fields and re-written the constructors for the same functionality. Original `SWPointer::invariant()` calls function `SuperWord::find_pre_loop_end()` for loop invariant checks. To help decoupling, we moved function `find_pre_loop_end()` to class `CountedLoopNode`. As function `SWPointer::Tracer::invariant_1()` is tightly coupled with `SuperWord` but only prints some debug messages, we temporarily removed it in this patch. We will consider adding it back after later refactoring of `SuperWord` so we added a `TODO` at its call site in this patch.
> 
> 3) We have a lot of memory phi node checks in loop optimizations. So we added a utility function `is_memory_phi()` in `node.hpp`.
> 
> Tested tier1~3 on x86 and AArch64. Also manually verified that option `VectorizeDebug` in compiler directives still works well.

@pfustc Thanks a lot for moving this code, and especially for untangling it from SuperWord. This will be helpful for many future vectorization projects.

I left a few comments, but otherwise this looks straight-forward and good to me.

src/hotspot/share/opto/vectorization.cpp line 40:

> 38: #endif
> 39: 
> 40: VPointer::VPointer(MemNode* mem, PhaseIdealLoop* phase, IdealLoopTree* lpt,

You could also call it `LPointer` or `LoopPointer`. `VPointer` sounds like `VectorPointer` - but it is not a pointer of a vector but a scalar memop. That could be confusing. But you could also argue it is a `VectorizationPointer`, and hence `VPointer.`

src/hotspot/share/opto/vectorization.cpp line 50:

> 48:   _nstack(nstack), _analyze_only(analyze_only), _stack_idx(0)
> 49: #ifndef PRODUCT
> 50:   , _tracer((phase->C->directive()->VectorizeDebugOption & 2) > 0)

You should also refactor the accessors for `VectorizeDebugOption`. I would move it from SuperWord to `vectorization.hpp/cpp` somehow. We should only do the "masking" `& 2` in one single place.

src/hotspot/share/opto/vectorization.cpp line 131:

> 129: bool VPointer::invariant(Node* n) const {
> 130:   NOT_PRODUCT(Tracer::Depth dd;)
> 131:   // TODO: Add more trace output for invariant check after later refactoring

We generally don't like `TODO`s in the code. Best is to just drop it in the code and file an RFE if you think it is really important.

When did this even trace anything?
`_slp->_lpt->is_member(_slp->_phase->get_loop(n_c)) != (int)_slp->in_bb(n)`

Do you think this tracing is relevant enough?

src/hotspot/share/opto/vectorization.cpp line 145:

> 143:         Node* n_c = phase()->get_ctrl(n);
> 144:         return phase()->is_dominator(n_c, pre_loop_end->loopnode());
> 145:       }

Is `pre_loop_end != nullptr` possible here? Before your patch we always found `_slp->pre_loop_head()`.
I'm just worried that if we do not find it, then we still return `is_not_member`, but `n` is still located in the space between pre and post loop.
What do you think about this?

And: would it make sense to cache the `pre_loop_head` in the `VPointer`?

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

PR Review: https://git.openjdk.org/jdk/pull/15013#pullrequestreview-1571704233
PR Review Comment: https://git.openjdk.org/jdk/pull/15013#discussion_r1290037785
PR Review Comment: https://git.openjdk.org/jdk/pull/15013#discussion_r1290023212
PR Review Comment: https://git.openjdk.org/jdk/pull/15013#discussion_r1290006470
PR Review Comment: https://git.openjdk.org/jdk/pull/15013#discussion_r1290015263