[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 # ">";
+}
----------------
david-arm wrote:
> paulwalker-arm wrote:
> > 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.
> Sadly it doesn't work because it prints out "p8" instead of "pn8", for example. The registers alias and so the function `getRegisterName` just returns "p8".
That's unfortunate. Thanks for checking.  Something to follow up on later that I've just noticed is `AArch64InstPrinter::printRegName` emits the register name wrapped in `<reg:...>` markup.  I'm not sure if we should be doing likewise for the `pn` registers.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:5680
+  case Match_InvalidSVEVecLenSpecifier:
+    return Error(Loc, "Invalid predicate-as-counter pattern");
   case Match_InvalidSVEExactFPImmOperandHalfOne:
----------------
How about "invalid vector length specifier, expected VLx2 or VLx4"?


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:7463
+AArch64AsmParser::tryParseSVEVecLenSpecifier(OperandVector &Operands) {
+  MCAsmParser &Parser = getParser();
+  int64_t Pattern;
----------------
Is this required?  It looks like `tryParseSVEPattern()` only did this to get access to `parseExpression()`, which this function doesn't need. Both `getTok()` and `Lex()` look directly accessible.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp:1782
+  if (Val > 1)
+    llvm_unreachable("Invalid Pattern");
+  if (auto Pat =
----------------
"vector length specifier"


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:45
+
+def sve_vec_len_specifier_enum_1b : Operand<i32>, TImmLeaf<i32, [{
+  return (((uint32_t)Imm) < 2);
----------------
Up to you but this isn't an immediate where the bit length is relevant so you could drop the `_1b`.


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

https://reviews.llvm.org/D136747