RFR: 8324890: C2 SuperWord: refactor out VLoop, make unrolling_analysis static, remove init/reset mechanism [v2]

[Vladimir Kozlov]

On Tue, 30 Jan 2024 20:28:07 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Subtask of https://github.com/openjdk/jdk/pull/16620
>> (The basic goal is to break SuperWord into different modules. This makes the code more maintainable and extensible. And eventually this allows some modules to be reused by other/new vectorizers.)
>> 
>> 1. Move out the shared code between `SuperWord::SLP_extract` (where we do vectorization) and `SuperWord::unrolling_analysis`, and move it to a new class `VLoop`. This allows us to decouple `unrolling_analysis` from the SuperWord object, and we can make it static.
>> 2. So far, SuperWord was reused for all loops in a compilation, and then "reset" (with `SuperWord::init`) for every loop. This is a bit of a nasty pattern. I now make a new `VLoop` and a new `SuperWord` object per loop.
>> 3. Since we now make more `SuperWord` objects, we allocate the internal data structures more often. Therefore, I now pre-allocate/reserve sufficient space on initialization.
>> 
>> Side-note about https://github.com/openjdk/jdk/pull/17604 (integrated, no need to read any more):
>> I would like to remove the use of `SuperWord::is_marked_reduction` from `SuperWord::unrolling_analysis`. For starters: it is not clear what it was ever good for. Second: it requires us to do reduction marking/analysis before `unrolling_analysis`, and hence makes the reduction marking shared between `unrolling_analysis` and vectorization. I could move the reduction marking to `VLoop` now. But the `_loop_reducitons` set would have to be put on an arena, and I would like to avoid creating an arena for the `unrolling_analysis`. Plus, it would just be nicer code, to have reduction analysis together with body analysis, type analysis, etc. and all of them in only in `SLP_extract`.
>
> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8324890
>  - _vtrace is moved to VLoop
>  - comment update
>  - cosmetics
>  - rename in preconditions
>  - remove loop_transform_helper
>  - fix small bug
>  - preallocate memory
>  - more refactoring
>  - moved mark_reductions
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/62978c80...25e3710e

In general it looks good. Few comments.

src/hotspot/share/opto/loopnode.cpp line 4866:

> 4864: 
> 4865:   // Auto-vectorize main-loop
> 4866:   if (C->do_superword() && C->has_loops() && !C->major_progress()) {

Should we rename `do_superword()` too?

src/hotspot/share/opto/loopopts.cpp line 4237:

> 4235:     cl->set_notpassed_slp();
> 4236:     cl->mark_do_unroll_only();
> 4237:   }

This is not related to vectorization. Can you move it back and execute based on return value from `autovectorize()` which is not used now?

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

PR Review: https://git.openjdk.org/jdk/pull/17624#pullrequestreview-1857348607
PR Review Comment: https://git.openjdk.org/jdk/pull/17624#discussion_r1474983887
PR Review Comment: https://git.openjdk.org/jdk/pull/17624#discussion_r1474993895