RFR: 8312332: C2: Refactor SWPointer out from SuperWord [v3]

[Emanuel Peter]

On Wed, 30 Aug 2023 11:53:38 GMT, Pengfei Li <pli at openjdk.org> wrote:

>> 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.`
>
> My idea is keeping the name as short as possible. I think it should be `VectorizationPointer` so `VPointer`. In my opinion, `LoopPointer` also sounds like a pointer to a loop but it isn't. I'd like to have more suggestions from other reviewers about the name.
> 
> BTW: `VPointer` is also used for vector memop in `SuperWord::output()`.

No worries, I can live with `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.
>
> Right, but doing this requires moving more handles including `phase` to `vectorization.hpp/cpp`. Perhaps we need to create a new class there first. I have created another task (https://bugs.openjdk.org/browse/JDK-8315361) and would like to do more refactoring later.

Ok, we can do that refactoring later.

>> 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`?
>
> Yes, I have found that `pre_loop_end` could be null when we construct `VPointer` in `SuperWord::output()` - see the code in `superword.cpp` (L2574, L2580). It's null because `CountedLoopNode::is_canonical_loop_entry()` returns null at this time. Before my patch, it cannot be null as we cached `_pre_loop_end` in the SuperWord class. To address your concern, my latest commit moves the cache of `_pre_loop_end` from `SuperWord` to `CountedLoopNode`. Some more asserts are also added to make sure it's used for main loops only. (Caching it in `VPointer` doesn't help).

Can you explain a bit more why `CountedLoopNode::is_canonical_loop_entry()` returned `null` at the time, and why you did move the caching?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15013#discussion_r1321084186
PR Review Comment: https://git.openjdk.org/jdk/pull/15013#discussion_r1321085185
PR Review Comment: https://git.openjdk.org/jdk/pull/15013#discussion_r1321094338