[PATCH] D105572: [AArch64][SME] Add load and store instructions

Wed Jul 14 04:59:12 PDT 2021

c-rhodes added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/regbank-inlineasm.mir:78
     ; CHECK-LABEL: name: inlineasm_virt_mixed_types
-    ; CHECK: INLINEASM &"mov $0, #0; mov $1, #0", 0 /* attdialect */, 655370 /* regdef:GPR32common */, def %0, 1245194 /* regdef:FPR64 */, def %1
+    ; CHECK: INLINEASM &"mov $0, #0; mov $1, #0", 0 /* attdialect */, 655370 /* regdef:GPR32common */, def %0, 1245194 /* regdef:WSeqPairsClass_with_sube32_in_MatrixIndexGPR32_12_15 */, def %1
     ; CHECK: [[COPY:%[0-9]+]]:gpr(s32) = COPY %0
----------------
aemerson wrote:
> c-rhodes wrote:
> > david-arm wrote:
> > > I realise adding a new class might have an effect on the other test files, but I'm not sure why this has changed from FPR64 to WSeqPairsClass_with_sube32_in_MatrixIndexGPR32_12_15 here?
> > > I realise adding a new class might have an effect on the other test files, but I'm not sure why this has changed from FPR64 to WSeqPairsClass_with_sube32_in_MatrixIndexGPR32_12_15 here?
> > 
> > Not entirely sure myself to be honest, not being familiar with GlobalISel. @aemerson any thoughts on the difference here? 
> I *think*, and please verify my theory here, that this is just because in this test we're hard coding an enum integer `1245194` for FPR64, as the operand for the INLINEASM instruction. If you add a new regclass that could be shifting the entries in the enum so `1245194` is no longer referencing FPR64. You should be able to just find the new integer value for FPR64 and change the test to use that.
> I *think*, and please verify my theory here, that this is just because in this test we're hard coding an enum integer `1245194` for FPR64, as the operand for the INLINEASM instruction. If you add a new regclass that could be shifting the entries in the enum so `1245194` is no longer referencing FPR64. You should be able to just find the new integer value for FPR64 and change the test to use that.

that seems to be the case, cheers @aemerson! I've took the new enum value for FPR64 from `llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll` and updated it here.

================
Comment at: llvm/test/MC/AArch64/SME/st1w-diagnostics.s:2
+// RUN: not llvm-mc -triple=aarch64 -show-encoding -mattr=+sme 2>&1 < %s| FileCheck %s
+
+// ------------------------------------------------------------------------- //
----------------
c-rhodes wrote:
> CarolineConcatto wrote:
> > Is it  possible to create a diagnostic test for Match_InvalidMatrixTileVectorH or Match_InvalidMatrixTileVectorV?
> > They are added in D105570, but I don't see it being used here.
> > Is it  possible to create a diagnostic test for Match_InvalidMatrixTileVectorH or Match_InvalidMatrixTileVectorV?
> > They are added in D105570, but I don't see it being used here.
> 
> The diagnostics for the matrix operands need some work to be honest, at the moment none of the invalid match types are triggered. Ideally the invalid tile test below would trigger:
> 
> ```  case Match_InvalidMatrixTileVectorH32:
>     return Error(Loc, "invalid matrix operand, expected za[0-3]h.s");```
> 
> but `za4h.s` isn't in `matchMatrixRegName` so `tryParseMatrixRegister` returns no match and `za4h.s` gets parsed as a token, at which point the operand parsing loop in `ParseIntruction` tries to parse a comma before the next operand, and since there isn't one separating the matrix operand and index, it errors, emitting: `'unexpected token in argument list'`.
> 
> I suppose we could treat `matchMatrixRegName` as an error and return `MatchOperand_ParseFail`. The diagnostic wouldn't be as accurate as what's added in D105570, but it would be an improvement over the current diagnostic which is emitted for the next operand (`[`).
> > Is it  possible to create a diagnostic test for Match_InvalidMatrixTileVectorH or Match_InvalidMatrixTileVectorV?
> > They are added in D105570, but I don't see it being used here.
> 
> The diagnostics for the matrix operands need some work to be honest, at the moment none of the invalid match types are triggered. Ideally the invalid tile test below would trigger:
> 
> ```  case Match_InvalidMatrixTileVectorH32:
>     return Error(Loc, "invalid matrix operand, expected za[0-3]h.s");```
> 
> but `za4h.s` isn't in `matchMatrixRegName` so `tryParseMatrixRegister` returns no match and `za4h.s` gets parsed as a token, at which point the operand parsing loop in `ParseIntruction` tries to parse a comma before the next operand, and since there isn't one separating the matrix operand and index, it errors, emitting: `'unexpected token in argument list'`.
> 
> I suppose we could treat `matchMatrixRegName` as an error and return `MatchOperand_ParseFail`. The diagnostic wouldn't be as accurate as what's added in D105570, but it would be an improvement over the current diagnostic which is emitted for the next operand (`[`).

Returning `MatchOperand_ParseFail` broke a bunch of unrelated tests so isn't really an option, but I've tweaked the `isMatrixRegOperand` predicate to return near match. The diagnostics added in D105570 are now used instead of `invalid operand for instruction` and I've added more diagnostics tests.

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

https://reviews.llvm.org/D105572