[PATCH] D130618: [AArch64][LoopVectorize] Enable tail-folding of simple loops on neoverse-v1

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @david-arm, I wonder if it makes sense to move some of these changes to a separate (NFC?) patch where you do the refactoring of the class, so that this patch becomes as simple as calling `setSVETailFoldingDefaultOpts(TFSimple);`. That way, if for any reason the patch needs to be reverted, we don't loose out on the refactoring. We can still test the current code with the llc options.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:43
 namespace {
 class TailFoldingKind {
 private:
----------------
Does this class still have any value? It only does bitwise |, & and ~, which you can inline below.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:60
+  uint8_t DefaultBits = TFDisabled;
+  std::string OrigVal;
+  SmallVector<StringRef, 4> TailFoldTypes;
----------------
nit: Could you rename this to UnparsedOptionString or something?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:60-61
+  uint8_t DefaultBits = TFDisabled;
+  std::string OrigVal;
+  SmallVector<StringRef, 4> TailFoldTypes;
+
----------------
sdesmalen wrote:
> nit: Could you rename this to UnparsedOptionString or something?
Rather than storing the string and substrings, can you just do the parsing as part of the constructor of TailFoldingOption so that you only need to store the bits?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:90
+      else
+        llvm_unreachable("No! That's impossible!");
+    }
----------------
Instead of an llvm_unreachable, do you want to print the error message here?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:112
+          TailFoldType != "norecurrences" && TailFoldType != "noreverse") {
         errs()
             << "invalid argument " << TailFoldType.str()
----------------
This is only printing to stderr. Should this be a fatal error?


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:537
+  TFReverse = 0x04,
+  TFSimple = 0x80,
+  TFAll = TFReductions | TFRecurrences | TFSimple | TFReverse
----------------
I know you're just moving this code around, but could you add a description of what 'simple' means? And maybe also add a description for TFRecurrences (are those first-order recurrences that reqequire a splice operation?)

(very minor nit: would it make sense to make this enum value `0x01` so that it's clear that anything not '1' is no longer a 'simple' loop?)


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

https://reviews.llvm.org/D130618