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

Wed Feb 14 14:41:09 UTC 2024

On Tue, 13 Feb 2024 16:26:25 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 with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - manual merge
>  - move _loop_or_ctrl from ResouceArena, bc ResourceMark in SuperWord::dependence_graph
>  - remove some comments
>  - VLoopTypes
>  - VLoopBody
>  - VLoopMemorySlices
>  - VLoopReductions
>  - 8325589

Nice refactoring into submodules! Here are some first comments. I have to do another iteration looking closer at the interplay of all the new submodules.

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

> 759:   // First, assign a dependence node to each memory node
> 760:   for (int i = 0; i < body().length(); i++ ) {
> 761:     Node *n = body().at(i);

Suggestion:

    Node* n = body().at(i);

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

> 766: 
> 767:   const GrowableArray<PhiNode*> &mem_slice_head = vloop_analyzer().memory_slices().heads();
> 768:   const GrowableArray<MemNode*> &mem_slice_tail = vloop_analyzer().memory_slices().tails();

Suggestion:

  const GrowableArray<PhiNode*>& mem_slice_head = vloop_analyzer().memory_slices().heads();
  const GrowableArray<MemNode*>& mem_slice_tail = vloop_analyzer().memory_slices().tails();

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

> 128:     if (state == VLoopAnalyzer::SUCCESS) {
> 129:     return true; // success
> 130:   }

I think it's more common to have the pattern "if (!foo()) print error and return". I therefore suggest to flip this:

  if (state != VLoopAnalyzer::SUCCESS) {
#ifndef PRODUCT
    if (vloop().is_trace_loop_analyzer()) {
      tty->print_cr("\nVLoopAnalyze::setup_submodules: failed: %s", state);
    }
#endif
    return false;
  }
  return true;

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

> 142:   // Skip any loop that has not been assigned max unroll by analysis.
> 143:   if (SuperWordLoopUnrollAnalysis && vloop().cl()->slp_max_unroll() == 0) {
> 144:     return VLoopAnalyzer::FAILURE_NO_MAX_UNROLL;

How about directly printing here instead of going over a failure string and returning false? You could, for example, have a method `trace_setup_submodules_failure(failure_string)`, and directly pass the string of `FAILURE_NO_MAX_UNROLL` here. Otherwise, I guess you still need to find this usage of `FAILURE_NO_MAX_UNROLL` here to understand exactly which conditions failed. By directly printing here, you have the entire context of the error.

Doing that would also allow the merge of `setup_submodules()` and `setup_submodules_helper()` into a single method.

It's just a suggestion - I leave it up to you to decide :-)

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

> 151:   _memory_slices.find_memory_slices();
> 152: 
> 153:   // If there is no memory slice detected, that means there is no store.

Suggestion:

  // If there is no memory slice detected, it means there is no store.

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

> 114: 
> 115:   bool is_trace_loop_analyzer() const {
> 116:     return vtrace().is_trace(TraceAutoVectorizationTag::LOOP_ANALYZER);

Any specific reason to not directly use `_vtrace` here instead of `vtrace()`? You also use the accessor method at other places inside the classes where you could also just use the fields. I think it improves the readability to use the fields directly such that you do not need to worry about side effects or other code inside the accessor methods.

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

> 233: 
> 234: public:
> 235:   // Find and mark reductions in a loop. Running mark_reductions() is similar to

I suggest to add new lines between the methods.

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

> 269:   GrowableArray<MemNode*> _tails;
> 270: 
> 271:   const VLoop& vloop() const { return _vloop; }

Is this method required? You could directly access the `VLoop` with `_vloop` inside the class.

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

> 461:   bool success() const { return _success; }
> 462: 
> 463:   Arena* arena()       { return &_arena; }

Since the `arena` is only for the submodules, do we really need to expose it here?

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

PR Review: https://git.openjdk.org/jdk/pull/17800#pullrequestreview-1879805273
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489554813
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489555331
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489184004
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489196826
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489200794
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489198749
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489557573
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489560942
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489207625