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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 4 inline comments as done.
c-rhodes added a subscriber: aemerson.
c-rhodes added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3992
+      return false;
+
+    SMLoc Loc = Parser.getTok().getLoc();
----------------
david-arm wrote:
> Should we be checking for '{' first and reporting an error if it's not? It could be '(', for example.
> Should we be checking for '{' first and reporting an error if it's not? It could be '(', for example.

Not sure what you mean here, if the current token isn't `{` then we wouldn't be here since the case is `case AsmToken::LCurly:`.


================
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
----------------
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? 


================
Comment at: llvm/test/MC/AArch64/SME/ld1b-diagnostics.s:4
+// ------------------------------------------------------------------------- //
+// Invalid tile (za0)
+
----------------
david-arm wrote:
> nit: Does 'za0' refer to what the tile should be? At first I thought `za0` was just typed wrong and should be `za1`.
> nit: Does 'za0' refer to what the tile should be? At first I thought `za0` was just typed wrong and should be `za1`.

Yeah za0 is what's valid, fair point though I can see how that could be confusing, I've clarified the comments by prefixing 'expected: '.


================
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
+
+// ------------------------------------------------------------------------- //
----------------
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 (`[`).


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

https://reviews.llvm.org/D105572