[PATCH] D105575: [AArch64][SME] Add zero instruction

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @c-rhodes, I'm only part-way through the patch, but here are some minor comments I have so far!



================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.td:1355
 
+class MatrixTileListAsmOperand<string RC, int EltSize> : AsmOperandClass {
+  let Name = "MatrixTileList" # EltSize;
----------------
This seems to be unused - can we delete the argument?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2063
+    else
+      for (auto OutReg : RegMap[std::make_pair(ElementWidth, Reg)])
+        OutRegs.insert(OutReg);
----------------
What if the pair doesn't make sense? For example, {16, AArch64::ZAD3}. In this case OutRegs will be returned empty.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2282
+    unsigned RegMask = getMatrixTileListRegMask();
+    unsigned MaxBits = 8;
+    for (unsigned I = MaxBits; I > 0; --I)
----------------
Given that you've now hard-coded properties about the maximum twice now (once above with RegMask <= 0xFF) and here with MaxBits = 8, is it worth having a constant declared somewhere that you can refer to? For example, MaxBits could be a constant in MatrixTileListOp and then

  RegMask = (1 << MaxBits ) - 1;


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2284
+    for (unsigned I = MaxBits; I > 0; --I)
+      OS << ((RegMask & (1 << (I - 1))) >> (I - 1));
+    break;
----------------
Do we need a separator here, like ' '?


================
Comment at: llvm/lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp:708
+                                                      const void *Decoder) {
+  if (RegMask > 0xFF)
+    return Fail;
----------------
Again, another hard-coded value here.


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

https://reviews.llvm.org/D105575