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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Overall, this looks pretty straightforward to me.

Apart from the few comments, I think the series is good up to here. Thanks!



================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:411
+
+    if (Kind == k_SVERegister)
+      return SVEReg.RegOpnd.RegNum;
----------------
sdesmalen wrote:
> rengolin wrote:
> > I'm assuming this will change back, as you're making this an actual `k_Register` with a different type, so I'll stop reviewing for now.
> Not sure I fully understand what you mean with 'changing back' into a k_Register, but to clarify my change, I  made it a separate k_SVERegister on purpose since it will be extended in future patches with attributes like shift/extend amount and element width.
The other patch confused me by using `k_SVERegister` alike. Now I see that you're moving things down the line. Np.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:844
+    return (Kind == k_SVERegister ||
+            (Kind == k_Register && Reg.Kind == RegKind::SVEDataVector)) &&
+           AArch64MCRegisterClasses[Class].contains(getReg());
----------------
To @javed.absar comment in D39088, this may be simplified by having all done at the RegKind level, instead of having multiple types of registers on top of multiple types of SVE vectors.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1833
     break;
+  case k_SVERegister:
+    OS << "<register " << getReg();
----------------
move this case together with `k_Register`


https://reviews.llvm.org/D39089