[PATCH] D136747: [AArch64][SVE2] Add the SVE2.1 cntp instruction

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.td:923
+    : PPRRegOp<Suffix, C, ElementSizeNone, RC> {
+  let PrintMethod = "printPredicateAsCounter<" # EltSize # ">";
+}
----------------
Do you need a special PrintMethod here? I likely missed this be reviewing your other patch but `printSVERegOp` (i.e. the default action for `PPRRegOp`) looks like it might just work.


================
Comment at: llvm/lib/Target/AArch64/AArch64SystemOperands.td:320
+
+class SVEPREDCNTPAT<string name, bits<1> encoding> : SearchableTable {
+  let SearchableFields = ["Name", "Encoding"];
----------------
Perhaps I'm being picky but I don't see this as a predicate pattern.  It's really a vector length specifier.


================
Comment at: llvm/lib/Target/AArch64/AArch64SystemOperands.td:325
+  string Name = name;
+  bits<1> Encoding;
+  let Encoding = encoding;
----------------
FYI: `bit` would work here also.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:5707-5714
+  case Match_InvalidSVEPNPredicateBReg:
+    return Error(Loc, "Invalid predicate register. Expected pn0.b..pn15.b");
+  case Match_InvalidSVEPNPredicateHReg:
+    return Error(Loc, "Invalid predicate register. Expected pn0.h..pn15.h");
+  case Match_InvalidSVEPNPredicateSReg:
+    return Error(Loc, "Invalid predicate register. Expected pn0.s..pn15.s");
+  case Match_InvalidSVEPNPredicateDReg:
----------------
Please agree on a consistent message style for these and the equivalent `_p8to15Reg` diagnostics.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:37
 
+class SVECntPatternOperand<int NumBits> : AsmOperandClass {
+  let Name = "SVECntPattern" # NumBits;
----------------
This patch alone doesn't suggest you need this complexity.  Is there something coming later that requires it?


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:9011
+
+class sve2p1_pcount_pn<string mnemonic, bits<2> sz, PNRRegOp pnrty>
+   : I<(outs GPR64:$Rd),
----------------
Please expose `opc` at this level and either name the class to match the encoding group or add a comment to say what the encoding group is.


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

https://reviews.llvm.org/D136747