[PATCH] D105570: [AArch64][SME] Add matrix register definitions and parsing support

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @c-rhodes, I've still got a few files to review, but I've left a few comments so far!



================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:385
+    unsigned RegNum;
+    int ElementWidth;
+    MatrixKind Kind;
----------------
I think this should be `unsigned` as well to match the definition of `getMatrixElementWidth`


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1510
 
+  template <MatrixKind Kind, int EltSize, unsigned RegClass>
+  DiagnosticPredicate isMatrixRegOperand() const {
----------------
`unsigned` to match `getMatrixElementWidth`?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2109
+  static std::unique_ptr<AArch64Operand>
+  CreateMatrixRegister(unsigned RegNum, int ElementWidth, MatrixKind Kind,
+                       SMLoc S, SMLoc E, MCContext &Ctx) {
----------------
`unsigned` here too I think


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3002
+  size_t DotPosition = Name.find('.');
+  assert(DotPosition != StringRef::npos && "Unexpected register");
+
----------------
Can this assert be hit easily in practice by simply typing the wrong assembly, i.e.

"addha za0hb"

?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3008
+
+  MatrixKind Kind = StringSwitch<MatrixKind>(RowOrColumn)
+                        .Case("h", MatrixKind::Row)
----------------
It looks like if I write assembly like this:

  addha za0p.d

then we'll treat this as a matrix tile. Do we have any tests that show us treating this type of register as an error?


================
Comment at: llvm/lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp:648
 
+static const SmallVector<SmallVector<unsigned, 15>, 4> MatrixZATileDecoderTable = {
+    {AArch64::ZAB0},
----------------
Should this be `5` instead of `4` to match the number of entries?


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

https://reviews.llvm.org/D105570