[PATCH] D39088: [AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:62
 
+enum class RegKind {Scalar, NeonVector, SVEVector };
+
----------------
Would it make sense to use something more generic for the names here, like `ScalableVector`?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:811
+  bool isReg() const override {
+    return Kind == k_Register && Reg.Kind == RegKind::Scalar;
+  }
----------------
Personally I think having predicate functions like `Reg.isScalar()`, `Reg.isNeonVector()` would be slightly more compact, but I think either way is fine.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:812
+    return Kind == k_Register && Reg.Kind == RegKind::Scalar;
+  }
+  bool isVectorReg() const {
----------------
>From looking at the functions around here, it seems like a space would be good here.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1889
 
+unsigned AArch64AsmParser::matchReqRegisterNameAlias(StringRef Name,
+                                                     RegKind Kind) {
----------------
What is the reason for moving this code out to a separate function? Is it to not change the interface of `matchRegisterNameAlias`, which still takes `bool isVector` as an argument?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1905
 unsigned AArch64AsmParser::matchRegisterNameAlias(StringRef Name,
                                                   bool isVector) {
   unsigned RegNum = isVector ? matchVectorRegName(Name)
----------------
I think keeping the boolean `isVector` argument has potential to confuse people. Any reason for not being explicit here and pass RegKind here? Because there are 2 vector register types now, `isVector` seems ambiguous.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1911
+  if (RegNum == 0)
+    RegNum = matchReqRegisterNameAlias (Name, Kind);
   return RegNum;
----------------
space between `matchRegisterNameAlias` and `()`?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2589
   Operands.push_back(
-      AArch64Operand::CreateReg(Reg, true, S, getLoc(), getContext()));
+      AArch64Operand::CreateReg(Reg, RegisterKind, S, getLoc(), getContext()));
+
----------------
Any reason for not passing `RegKind::NeonVector` directly here? It seems like RegisterKind is not used anywhere else around here.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2590
+      AArch64Operand::CreateReg(Reg, RegisterKind, S, getLoc(), getContext()));
+
   // If there was an explicit qualifier, that goes on as a literal text
----------------
stray new line?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2771
+      FirstReg, Count, NumElements, ElementKind, S, getLoc(),
+      getContext()));
 
----------------
unrelated change?


https://reviews.llvm.org/D39088