[PATCH] D136088: [AArch64]SME2 instructions that use ZTO operand

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1380
+// True if the immediate is a multiple of 8 in the range [0,56].
+def UImm3s8Operand : UImmScaledMemoryIndexed<3, 8>;
+
----------------
Is this really a memory index? It looks like an offset into the ZT0 vector, so should just be a normal imm/index operand.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1384
+[{ return Imm >= 0 && Imm <= 56 && ((Imm % 8) == 0); }], UImmS8XForm> {
+  let PrintMethod = "printImmScale<8, false>";
+  let ParserMatchClass = UImm3s8Operand;
----------------
I imagine it's not just a simple replacement but as a starting point perhaps use `printVectorIndex`, given this is how the immediate is used.


================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.td:1300
 
+def ZT0R : RegisterClass<"AArch64", [untyped], 512, (add ZT0)> {
+  let Size = 512;
----------------
Just an idea but what about calling the register class `ZT`, which currently contains the single register `ZT0`?


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:32
+
+// M = SME array register
+// P = Predicate register
----------------
Perhaps add `(ZA)`?

I think it's worth just committing this comment separately into main given it's already in use by already committed patches.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3871-3877
+    const auto &Op = static_cast<AArch64Operand *>(Operands.back().get());
+    if (Op->isReg() && Op->getReg() == AArch64::ZT0 &&
+        getTok().is(AsmToken::LBrac))
+      // There's no comma after indexed ZT0 register, so we can parse the next
+      // operand immediately.
+      return parseOperand(Operands, false, false);
     return false;
----------------
It has been a while since I've had to do this but I suspect you really want you're own `ParserMethod/tryParse...` method if the existing ones don't already work for you.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4089-4092
+    if (RegTok.is(AsmToken::Identifier) && ParseRes == MatchOperand_NoMatch &&
+        (RegTok.getString().startswith_insensitive("za") ||
+         RegTok.getString().equals_insensitive("zt0")))
+      return MatchOperand_NoMatch;
----------------
My nativity showing here but how does `zt0` related to vector lists? and why do you need extra handling for `za` given this patch relates to `zt0`?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:5653
                       "and with matching element types");
+  case Match_InvalidZT0:
+    return Error(Loc, "operand must be zt0 register");
----------------
`Match_InvalidLookupTable`?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:5654
+  case Match_InvalidZT0:
+    return Error(Loc, "operand must be zt0 register");
   default:
----------------
To match similar ZA diagnostics how about `invalid lookup table, expected zt0`?


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp:1218-1226
+template <int Scale, bool PrintHash>
 void AArch64InstPrinter::printImmScale(const MCInst *MI, unsigned OpNum,
                                        const MCSubtargetInfo &STI,
                                        raw_ostream &O) {
-  O << markup("<imm:") << '#'
-    << formatImm(Scale * MI->getOperand(OpNum).getImm()) << markup(">");
+  if (PrintHash) {
+    O << markup("<imm:") << '#'
+      << formatImm(Scale * MI->getOperand(OpNum).getImm()) << markup(">");
----------------
To me this looks like a sign you're using the wrong print routine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136088