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

[Bryan Chan via Phabricator via llvm-commits]

bryanpkc added a comment.

Hi @c-rhodes, we had done some design work for the `ZERO` instruction, and it is interesting to see your implementation. I have some questions about the code, based on my understanding of the ISA.



================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.td:1373
+def MatrixTileList32 : MatrixTileListOperand<"MPR32", 32, (ops MPR32)>;
+def MatrixTileList64 : MatrixTileListOperand<"MPR64", 64, (ops MPR64)>;
+
----------------
Does this restrict a matrix tile list to contain only tiles of the same element type? The ISA documentation doesn't impose such a restriction, so I think it would be legal to write something like `zero { za0.s, za5.d }`. Did you consider supporting matrix tile lists of mixed element types?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3870
+    if (ElementWidth != NextElementWidth) {
+      Error(Loc, "mismatched register size suffix");
+      return MatchOperand_ParseFail;
----------------
Is this specified in the ISA documentation? I thought matrix tile lists of mixed element types were allowed.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3876
+    if (RI->getEncodingValue(Reg) <= (RI->getEncodingValue(PrevReg))) {
+      Error(Loc, "registers must be sequential");
+      return MatchOperand_ParseFail;
----------------
Is this specified in the ISA documentation? It isn't obvious that the tiles in the list must be ordered.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:661
+class sme_zero_inst<string mnemonic>
+    : I<(outs MatrixTileList64:$imm), (ins),
+        mnemonic, "\t$imm", "", []>, Sched<[]> {
----------------
Wouldn't it be better for the instruction to accept an output `RegisterOperand` that covers all possible tile types, which would allow register allocation to assign the appropriate tile registers? Doing so would also allow us to distinguish between `zero { za0.d, za4.d }` and `zero { za0.s }` when we need to parse assembly code back into machine IR with correct register semantics.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:688
+  def : InstAlias<"zero\t\\{za1.s,za2.s\\}", (!cast<Instruction>(NAME) 0b01100110), 1>;
+  def : InstAlias<"zero\t\\{za2.s,za3.s\\}", (!cast<Instruction>(NAME) 0b11001100), 1>;
+  def : InstAlias<"zero\t\\{za0.s,za1.s,za2.s\\}", (!cast<Instruction>(NAME) 0b01110111), 1>;
----------------
david-arm wrote:
> Are we missing ones for {za1.s,za3.s} and {z0.s,za2.s}?
Why are these implemented as `InstAlias`? Can they not be parsed by `AArch64AsmParser::tryParseMatrixTileList`?


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

https://reviews.llvm.org/D105575