[PATCH] D130618: [AArch64][LoopVectorize] Enable tail-folding by default for SVE

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:62-63
+        add(TFAll);
       else if (TailFoldType == "default")
-        Bits = 0; // Currently defaults to never tail-folding.
+        NeedsDefault = true;
       else if (TailFoldType == "simple")
----------------
This class seems to be mixing two concepts:
* Features that the loop requires for tail folding (i.e. recurrences/reductions/anything-else).
* What the target wants as the default and the parsing-logic for user-visible toggles.

This makes it quite tricky to follow the logic, especially the need for both `NeedsDefault` + `DefaultBits` and Add/RemoveBits`.

Perhaps it would make more sense to have two classes:

  class TailFoldingKind;    // This encodes which features the loop requires
  class TailFoldingOption;  // This encodes the default (which itself will be a TailFoldingKind object), and has logic to parse strings.

Then you can add a method such as `TailFoldingKind TailFoldingOption::getSupportedMode() const { .. }` that you can use to query if the target's mode satisfies the required TailFoldingKind from the Loop.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:95-99
 
-  void add(uint8_t Flag) { Bits |= Flag; }
-  void remove(uint8_t Flag) { Bits &= ~Flag; }
+  void remove(uint8_t Flags) {
+    RemoveBits |= Flags;
+    AddBits &= ~Flags;
+  }
----------------
Perhaps I'm missing something, but this logic with `AddBits` and `RemoveBits` seems unnecessarily complicated. Can't you have a single bitfield and do something like this:

  void set(uint8_t Flags, bool Enable) {
    if (Enable)
      Bits |= Flags;
    else
      Bits &= ~Flags;
  }

?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:3057-3058
 
   TailFoldingKind Required; // Defaults to 0.
+  Required.setDefault(0);
   if (LVL->getReductionVars().size())
----------------
This seems redundant given the line above that says 'Defaults to 0'.

How about creating a constructor for it that takes a TailFoldingOpts as operand, so that you can write `TailFoldingKind Required(TFDisabled)`?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:3066
 
   return (TailFoldingKindLoc & Required) == Required;
 }
----------------
Is it worth creating a method for this, so that you don't have to expose the bit-field to users, e.g.

  return TailFoldingKindLoc.satisfies(Required);

?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130618/new/

https://reviews.llvm.org/D130618