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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 2 inline comments as done.
c-rhodes added a comment.

In D105570#2870746 <https://reviews.llvm.org/D105570#2870746>, @david-arm wrote:

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

thanks for reviewing Dave



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


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

No, since that assembly won't match in `matchMatrixRegName` so it should return no match above.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3008
+
+  MatrixKind Kind = StringSwitch<MatrixKind>(RowOrColumn)
+                        .Case("h", MatrixKind::Row)
----------------
david-arm wrote:
> 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?
> 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?

Same as above, that's not a valid reg name so it shouldn't match.


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

Yeah good spot, both values were wrong, 15 should be 16.


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

https://reviews.llvm.org/D105570