RFR: 8325589: C2 SuperWord refactoring: create VLoopAnalyzer with Submodules [v6]

[Christian Hagedorn]

On Thu, 15 Feb 2024 22:57:15 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Subtask of https://github.com/openjdk/jdk/pull/16620
>> 
>> **Goals**:
>> - **modularity**: SuperWord is a nasty large class, and it is difficult to know what impacts what.
>> - **code reuse**: In the future, we may want to reuse some components of SuperWord in other Vectorizers. For example the post-loop-vectorizer ([JDK-8308994](https://bugs.openjdk.org/browse/JDK-8308994)).
>> 
>> Hence, I pull some parts out of SuperWord, and put them in a new class `VLoopAnalyzer`, with 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.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   indentation

Thanks for the updates! Looks good now. I have only some code style comments left which can mostly just directly be applied in the PR.

> We can discuss if I should revert igvn, phase, cl, ... to fields as well. But that change was in a different changeset, and so I would also do that change in a future RFE.

I agree. Maybe it makes sense to do a pass over the entire new code again once all refactoring is done. Then we can fix such things and also left-overs like missing `const` etc.

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

> 862: 
> 863: // Get all memory nodes of a slice, in reverse order
> 864: void VLoopMemorySlices::get_slice(PhiNode* head, MemNode* tail, GrowableArray<Node*> &slice) const {

Should we mention in the method name that we return the nodes in reverse order?

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

> 863: // Get all memory nodes of a slice, in reverse order
> 864: void VLoopMemorySlices::get_slice(PhiNode* head, MemNode* tail, GrowableArray<Node*> &slice) const {
> 865:   assert(slice.length() == 0, "start empty");

While at it, you could use:
Suggestion:

  assert(slice.is_empty(), "start empty");

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

> 2275:   GrowableArray<Node*> old_last_store_in_slice(max_slices, max_slices, nullptr);
> 2276: 
> 2277:   const GrowableArray<PhiNode*> &mem_slice_head = _vloop_analyzer.memory_slices().heads();

Suggestion:

  const GrowableArray<PhiNode*>& mem_slice_head = _vloop_analyzer.memory_slices().heads();

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

> 2932: // Return nullptr if success, else failure message
> 2933: VStatus VLoopBody::construct() {
> 2934:   assert(_body.length() == 0, "body is empty");

Suggestion:

  assert(_body.is_empty(), "body is empty");

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

> 3203:     if (nn->is_Cmp() && nn->in(0) == nullptr) {
> 3204:       assert(_vloop.in_bb(nn->in(1)) || _vloop.in_bb(nn->in(2)),
> 3205:              "one of the inputs must be in the loop too");

Suggestion:

             "one of the inputs must be in the loop, too");

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

> 150:   // If there is no reduction and no store, then we give up, because
> 151:   // vectorization is not possible anyway (given current limitations).
> 152:   if (!reductions().is_marked_reduction_loop() &&

Since you do `_reductions.mark_reductions()` above, I suggest to also call this on `_reductions` to make it clear that it's the same thing within the context of this method.
Suggestion:

  if (!_reductions.is_marked_reduction_loop() &&

src/hotspot/share/opto/vectorization.hpp line 307:

> 305: 
> 306:   const GrowableArray<PhiNode*> &heads() const { return _heads; }
> 307:   const GrowableArray<MemNode*> &tails() const { return _tails; }

Suggestion:

  const GrowableArray<PhiNode*>& heads() const { return _heads; }
  const GrowableArray<MemNode*>& tails() const { return _tails; }

src/hotspot/share/opto/vectorization.hpp line 310:

> 308: 
> 309:   // Get all memory nodes of a slice, in reverse order
> 310:   void get_slice(PhiNode* head, MemNode* tail, GrowableArray<Node*> &slice) const;

Suggestion:

  void get_slice(PhiNode* head, MemNode* tail, GrowableArray<Node*>& slice) const;

src/hotspot/share/opto/vectorization.hpp line 334:

> 332:   // Mapping node->_idx -> body_idx
> 333:   // Can be very large, and thus lives in VSharedData
> 334:   GrowableArray<int>&  _body_idx;

Suggestion:

  GrowableArray<int>& _body_idx;

src/hotspot/share/opto/vectorization.hpp line 466:

> 464: 
> 465: public:
> 466:   VLoopAnalyzer(const VLoop& vloop, VSharedData &vshared) :

Suggestion:

  VLoopAnalyzer(const VLoop& vloop, VSharedData& vshared) :

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17800#pullrequestreview-1900140722
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502163602
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502159793
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502165829
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502166694
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502169369
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502183242
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502195264
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502195397
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502195744
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1502201119