RFR: 8317572: C2 SuperWord: refactor/improve TraceSuperWord, replace VectorizeDebugOption with TraceAutoVectorization

[Christian Hagedorn]

On Fri, 26 Jan 2024 12:49:50 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> Subtask of https://github.com/openjdk/jdk/pull/16620
> 
> I got approval to remove VectorizeDebugOption: [JDK-8320668](https://bugs.openjdk.org/browse/JDK-8320668)
> 
> I want a more general flag for AutoVectorization, that can trace different components of AutoVectorization.
> It should be a CompileCommand, so that it can select which methods it traces for.
> 
> TraceSuperWord should still look similar, and select a subset of the TraceAutoVectorization components (those for SuperWord), but still apply to all classes/methods.
> 
> With more refactoring later in [JDK-8315361](https://bugs.openjdk.org/browse/JDK-8315361), this flag should become more usable and interpretable. Especially, the idea is that different components of the `VLoop / VLoopAnalyzer` can have tracing enabled / disabled.
> 
> **How to use the flag:**
> Get "help", i.e. see all available tags:
> `./java -Xcomp -XX:CompileCommand=TraceAutoVectorization,*::*,help --version`
> 
> See "rejections" (i.e. failures where we don't vectorize) and successes (using TraceNewVectors):
> `./java -Xcomp -XX:CompileCommand=TraceAutoVectorization,*::*,SW_REJECTIONS -XX:+TraceNewVectors --version`
> The results are currently underwhealming. I will have to track many more failures, and I will do that with the bigger refactoring, when I move around the code and require error code returning everywhere, and then I can use that error code for printing.

Generally looks good! I have some comments.

src/hotspot/share/compiler/compilerDirectives.cpp line 305:

> 303:   _directive(d),
> 304:   _ideal_phase_name_set(PHASE_NUM_TYPES, mtCompiler),
> 305:   _traceautovectorization_tags(TRACEAUTOVECTORIZATION_TAG_NUM, mtCompiler)

I also suggest to use underlines and at other places to separate the words trace, auto, and vectorization:
Suggestion:

  _trace_auto_vectorization_tags(TRACE_AUTO_VECTORIZATION_TAG_NUM, mtCompiler)

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

> 923: void SuperWord::mem_slice_preds(Node* start, Node* stop, GrowableArray<Node*> &preds) {
> 924:   assert(preds.length() == 0, "start empty");
> 925:   Node* n = start;

There is still a usage of `TraceSuperWord` on L927. Should this also be replaced?

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

> 1254: 
> 1255: #ifndef PRODUCT
> 1256:   if(is_trace_superword_alignment()) {

Suggestion:

  if (is_trace_superword_alignment()) {

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

> 1278:         _packset.append(pair);
> 1279: #ifndef PRODUCT
> 1280:         if(is_trace_superword_alignment()) {

Suggestion:

        if (is_trace_superword_alignment()) {

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

> 1305:   int align = alignment(s1);
> 1306: #ifndef PRODUCT
> 1307:   if(is_trace_superword_alignment()) {

Suggestion:

  if (is_trace_superword_alignment()) {

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

> 1352:     _packset.append(pair);
> 1353: #ifndef PRODUCT
> 1354:     if(is_trace_superword_alignment()) {

Suggestion:

    if (is_trace_superword_alignment()) {

src/hotspot/share/opto/superword.hpp line 282:

> 280:     return TraceSuperWord ||
> 281:            _vtrace.is_trace(TraceAutoVectorizationTag::SW_PRECONDITION);
> 282:   }

I suggest to add new lines between the methods

src/hotspot/share/opto/traceAutoVectorizationTag.hpp line 31:

> 29: 
> 30: // TODO: adjust tags to what we need
> 31: #define COMPILER_TRACEAUTOVECTORIZATION_TAG(flags) \

I suggest to use underlines, same for `TRACEAUTOVECTORIZATION_TAG_NUM/NONE`:
Suggestion:

#define COMPILER_TRACE_AUTO_VECTORIZATION_TAG(flags) \

src/hotspot/share/opto/traceAutoVectorizationTag.hpp line 41:

> 39:   flags(SW_REJECTIONS,        "Trace SuperWord rejections (non vectorizations)") \
> 40:   flags(SW_PACKSET,           "Trace SuperWord packset at different stages") \
> 41:   flags(SW_INFO,              "Trace SuperWord info") \

Maybe mention here that this tag prints some general info + the most important SW tags

src/hotspot/share/opto/traceAutoVectorizationTag.hpp line 42:

> 40:   flags(SW_PACKSET,           "Trace SuperWord packset at different stages") \
> 41:   flags(SW_INFO,              "Trace SuperWord info") \
> 42:   flags(SW_VERBOSE,           "Trace SuperWord verbose (all)") \

Maybe mention here that verbose prints all SW tags

src/hotspot/share/opto/traceAutoVectorizationTag.hpp line 75:

> 73: }
> 74: 
> 75: class TraceAutoVectorizationTagNameIter {

This class is almost identical to `PhaseNameIter`. Can the code somehow be shared?

src/hotspot/share/opto/traceAutoVectorizationTag.hpp line 144:

> 142:         set_bit = false;
> 143:       }
> 144:       TraceAutoVectorizationTag tat = find_tag(tag_name);

`tat` is not very intuitive when I've read the code below. I suggest to go with tag as it should be clear what kind of tags we mean in this context.
Suggestion:

      TraceAutoVectorizationTag tag = find_tag(tag_name);

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

PR Review: https://git.openjdk.org/jdk/pull/17586#pullrequestreview-1845862666
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467721995
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467775132
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467726494
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467726661
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467726799
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467726943
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467736522
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467720837
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467783144
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467752599
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467733616
PR Review Comment: https://git.openjdk.org/jdk/pull/17586#discussion_r1467758954