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

[Emanuel Peter]

On Wed, 14 Feb 2024 14:38:25 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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.

Thanks @chhagedorn for all the comments. I think I addressed everything now. But let me know if you insist on something I now decided not to do.

> 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;

Done.

> 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.

done.

> 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.

I think this is a bit of a style question. I prefer these accessors, because it makes refactoring easier. You can just replace a field by a call to some other method later. And I make sure to have all the accessors be `const`. So side-effects should really not be happening, unless someone hacks the `const` away...

> 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.

Good idea. In SuperWord everything was ultra compact, but we can do things different now :)

> 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.

I prefer it this way. It also keeps the code a bit more consistent everywhere: all places use the accessor method and then refactorings are simple: just replace what is in the accessor, rather than change it at every use-site.

> 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?

Nice catch! It was actually never used.

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

PR Comment: https://git.openjdk.org/jdk/pull/17800#issuecomment-1944151827
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489726246
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489723172
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489720208
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489728861
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489730559
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1489727017