[PATCH] D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.td:37
+  def zsub    : SubRegIndex<128>;
+  def zsub_hi : SubRegIndex<128>; // Note: Should never be used.
   // Note: Code depends on these having consecutive numbers
----------------
Maybe add a quick note WHY it should never be used?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:85
   int tryMatchVectorRegister(StringRef &Kind, bool expected);
+  int tryParseSVEVectorRegister(const AsmToken &Tok, StringRef &Kind);
   bool parseRegister(OperandVector &Operands);
----------------
Will this function also be used to parse SVE predicate registers? If not, it's probably clearer to use SVEDataVectorRegister in the name?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:187
     k_ShiftExtend,
+    k_SVERegister,
     k_FPImm,
----------------
Will we have a separate kind for predicate registers? Or should this be SVEDataRegister too?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:274
 
+  struct SVERegOp {
+    int ElementWidth;
----------------
SVEDataRegOp? I think if we go with SVEDataRegister and SVEPredicateRegister, we should be consistent.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3270
 
+static inline bool isMatchingOrAlias(unsigned ZReg, unsigned Reg) {
+  assert((ZReg >= AArch64::Z0) && (ZReg <= AArch64::Z31));
----------------
where is this used?


https://reviews.llvm.org/D39089