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

[Emanuel Peter]

On Wed, 14 Feb 2024 16:04:07 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

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

I am now using `_vtrace`.

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

I removed these cases now

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1491784218
PR Review Comment: https://git.openjdk.org/jdk/pull/17800#discussion_r1491783842