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

[Cullen Rhodes via Phabricator via llvm-commits]

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


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

I've change the match register name function (`matchMatrixTileListRegName`) to only match tiles that are valid for tile lists, now we shouldn't end up here I've added an assert that the pair isn't empty.


================
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;
----------------
david-arm wrote:
> Do we need a separator here, like ' '?
> Do we need a separator here, like ' '?

I don't think so, this just emits the bits, e.g.

`zero    {za0.s, za2.s} -> <matrixlist 01010101`

although I realise now the closing `>` is missing, I'll fix that.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3806
+
+    unsigned RegNum = matchMatrixTileRegName(Name);
+    if (!RegNum)
----------------
david-arm wrote:
> Is 'RegNum' guaranteed to always be > 0 for a valid register?
> Is 'RegNum' guaranteed to always be > 0 for a valid register?

Yeah 0 is NoRegister, defined in `llvm/include/llvm/MC/MCRegister.h`:
`static constexpr unsigned NoRegister = 0u;`

I suppose the match register functions could default on NoRegister to be more explicit, but not sure how useful that is.


================
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;
----------------
sdesmalen wrote:
> bryanpkc wrote:
> > c-rhodes wrote:
> > > bryanpkc wrote:
> > > > Is this specified in the ISA documentation? It isn't obvious that the tiles in the list must be ordered.
> > > > Is this specified in the ISA documentation? It isn't obvious that the tiles in the list must be ordered.
> > > 
> > > No, but I'm not sure there's a good reason for the tiles to not be in order?
> > IMO the assembler should be more forgiving here, e.g. when the user programmatically generates the assembly code and forgets to sort the registers. We may also want to be consistent with the ARM backend, which produces a warning, not an error, in a similar situation:
> > ```
> >         .text
> > foo.s:1:14: warning: register list not in ascending order
> > stm sp!, {r5,r3,r0}
> >              ^
> > foo.s:1:17: warning: register list not in ascending order
> > stm sp!, {r5,r3,r0}
> >                 ^
> >         stm     sp!, {r0, r3, r5}
> > ```
> > 
> I agree that if the spec doesn't require the operands to be in a specific order, that the instruction should accept the operands in any order.
> IMO the assembler should be more forgiving here, e.g. when the user programmatically generates the assembly code and forgets to sort the registers. We may also want to be consistent with the ARM backend, which produces a warning, not an error, in a similar situation:
> ```
>         .text
> foo.s:1:14: warning: register list not in ascending order
> stm sp!, {r5,r3,r0}
>              ^
> foo.s:1:17: warning: register list not in ascending order
> stm sp!, {r5,r3,r0}
>                 ^
>         stm     sp!, {r0, r3, r5}
> ```
> 

Fair point, I've changed it to a warning.


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

not really sure where I could put a constant here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105575