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

[Emanuel Peter]

On Wed, 14 Feb 2024 10:20:17 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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 :-)
>
> Another thought here: Shouldn't this check here already be performed in `VLoop::check_preconditions_helper()`?

As discussed offline:
the main confusion factor was the `const char*`. I now use `VStatus` instead. And I don't want to just return a boolean. I want to force us to log failure messages consistently. And if you have to return a message for failures, then that is automatic.

The check could maybe be done before VLoopAnalyzer. But I hope to get rid of the whole unrolling analysis at some point, when I implement vector-widening. So for now I'll leave it where it was previously.

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

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