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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
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;
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/SMEInstrFormats.td:661
+class sme_zero_inst<string mnemonic>
+    : I<(outs MatrixTileList64:$imm), (ins),
+        mnemonic, "\t$imm", "", []>, Sched<[]> {
----------------
c-rhodes wrote:
> bryanpkc wrote:
> > c-rhodes wrote:
> > > bryanpkc wrote:
> > > > 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.
> > > > 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.
> > > 
> > > I've not given that a great deal of thought yet to be honest, our focus is on MC layer support at the moment, may revisit this in the future.
> > We also went with an immediate operand at first, but eventually replaced it with a register operand, mainly to allow register allocation to work.
> > We also went with an immediate operand at first, but eventually replaced it with a register operand, mainly to allow register allocation to work.
> 
> Your approach sounds more complete, I'd be interested in taking a look, are you able to upstream it?
@bryanpkc you make a good point and it would be interested to see those patches!

For this patch I think that unless the changes you're suggesting are trivial, it would make sense to have any changes that are not required for the assembler as follow-up patches. I'm a bit cautious about this otherwise holding up SME asm support into LLVM 13, since those changes aren't necessarily required for the assembler.


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

https://reviews.llvm.org/D105575