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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @c-rhodes, I've not finished reviewing all the tests and instr format td changes yet, but here are some comments so far!



================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3026
+
+  if (getLexer().is(AsmToken::LBrac))
+    // There's no comma after matrix operand, so we can parse the next operand
----------------
nit: Just for readability is worth putting braces {} around the outer if-block?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3716
         ParseRes == MatchOperand_ParseFail ||
         (ParseRes == MatchOperand_NoMatch && NoMatchIsError)) {
       Error(Loc, "vector register expected");
----------------
I think you can avoid the if block above entirely if you simply add an extra condition here, right? For example,

  (ParseRes == MatchOperand_NoMatch && NoMatchIsError && !RegTok.getString().startswith_insensitive("za"))

That way we just fall through to the default and return MatchOperand_NoMatch, which is what you want I think? It feels like the new if block exists simply to stop the if block below from returning MatchOperand_ParseFail.


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


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp:527
+  auto RegOpnd = MI.getOperand(OpIdx).getReg();
+  return RegOpnd - AArch64::W12;
+}
----------------
Is it worth asserting that RegOpnd >= AArch64::W12 here?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105572