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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17976
-  switch (VT.getVectorNumElements()) {
-  default:
-    llvm_unreachable("unexpected element count for SVE predicate");
----------------
You're removing the unreachable here. Perhaps it's worth adding an assert:

  assert(PgPattern && "Unexpected element count for SVE predicate")


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18007
-
-  // TODO: For vectors that are exactly getMaxSVEVectorSizeInBits big, we can
-  // use AArch64SVEPredPattern::all, which can enable the use of unpredicated
----------------
Maybe we should leave this TODO here as I think it still applies?


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:457
 
+inline static unsigned AArch64SVEPredPatternToElementCount(unsigned Pattern) {
+  unsigned MinNumElts = 0;
----------------
I think that using both inline and static keywords here causes clang-tidy to think it's unused. Can you just use the `inline` keyword here instead?


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:457
 
+inline static unsigned AArch64SVEPredPatternToElementCount(unsigned Pattern) {
+  unsigned MinNumElts = 0;
----------------
david-arm wrote:
> I think that using both inline and static keywords here causes clang-tidy to think it's unused. Can you just use the `inline` keyword here instead?
I think this breaks case style rules - perhaps this should be named something like getNumElementsFromSVEPredPattern?


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:492
+inline static unsigned
+AArch64ElementCountToSVEPredPattern(unsigned MinNumElts) {
+  unsigned PgPattern;
----------------
Same comments as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108705