[PATCH] D108705: [AArch64][SVE] Add API for conversion between SVE predicate pattern and element count. NFC

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17974
 
-  int PgPattern;
-  switch (VT.getVectorNumElements()) {
-  default:
-    llvm_unreachable("unexpected element count for SVE predicate");
-  case 1:
-    PgPattern = AArch64SVEPredPattern::vl1;
-    break;
-  case 2:
-    PgPattern = AArch64SVEPredPattern::vl2;
-    break;
-  case 4:
-    PgPattern = AArch64SVEPredPattern::vl4;
-    break;
-  case 8:
-    PgPattern = AArch64SVEPredPattern::vl8;
-    break;
-  case 16:
-    PgPattern = AArch64SVEPredPattern::vl16;
-    break;
-  case 32:
-    PgPattern = AArch64SVEPredPattern::vl32;
-    break;
-  case 64:
-    PgPattern = AArch64SVEPredPattern::vl64;
-    break;
-  case 128:
-    PgPattern = AArch64SVEPredPattern::vl128;
-    break;
-  case 256:
-    PgPattern = AArch64SVEPredPattern::vl256;
-    break;
-  }
-
-  // TODO: For vectors that are exactly getMaxSVEVectorSizeInBits big, we can
-  // use AArch64SVEPredPattern::all, which can enable the use of unpredicated
-  // variants of instructions when available.
+  unsigned PgPattern =
+      AArch64ElementCountToSVEPredPattern(VT.getVectorNumElements());
----------------
This'll need to be validated as with the original code.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18007-18009
-  // TODO: For vectors that are exactly getMaxSVEVectorSizeInBits big, we can
-  // use AArch64SVEPredPattern::all, which can enable the use of unpredicated
-  // variants of instructions when available.
----------------
I think this is an important comment that should remain within `getPredicateForFixedLengthVector`. See comment on D108706.


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:457
 
+inline static unsigned AArch64SVEPredPatternToElementCount(unsigned Pattern) {
+  unsigned MinNumElts = 0;
----------------
I'm not a fan of the names of these functions as `ElementCount` has an existing meaning within LLVM and these function are not really honouring it. Perhaps `getSVEPredicatePatternAsFixedLength/getSVEPredicatePatternForFixedLength`? Or something of similar ilk.

Can you add a function comment that makes it explicit that zero is returned for the case where no translation is possible.


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:470-471
+  case AArch64SVEPredPattern::vl8:
+    MinNumElts = Pattern;
+    break;
+  case AArch64SVEPredPattern::vl16:
----------------
Is there an LLVM coding style policy here? For example is setting `MinNumElts` and breaking preferred over just using return.


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:498-499
+  case 1:
+    PgPattern = AArch64SVEPredPattern::vl1;
+    break;
+  case 2:
----------------
Same "return" comment as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108705