[PATCH] D61513: [AArch64][SVE2] Add SVE2 target features to backend and TargetParser

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: lib/Support/AArch64TargetParser.cpp:100
+  if (Extensions & AEK_BITPERM)
+    Features.push_back("+bitperm");
   if (Extensions & AEK_RCPC)
----------------
rovka wrote:
> It might be a bit long, but I'd prefer sve2-bitperm here, to avoid the kind of situation we have in the ARM backend with hwdiv / hwdiv-arm.
It was decided to name this `bitperm`  to avoid the situation where any potential future SVE2 extensions will be enabled with `sve2-xxx` as this will make the -march longer and so far architecture features have not been prefixed with arch level or vector extension name.

I agree with your concern regarding the hwdiv / hwdiv-arm, I will raise this but it's worth mentioning it's not trivial to change at this point as Binutils are moving forward with the same flag names.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:118
+                                 AssemblerPredicate<"FeatureSVE2SHA3", "sve2-sha3">;
+def HasSVE2BitPerm   : Predicate<"Subtarget->hasSVE2()">,
+                                 AssemblerPredicate<"FeatureSVE2BitPerm", "bitperm">;
----------------
rovka wrote:
> This should be Subtarget->hasSVE2BitPerm(), right?
Yeah, good spot!


================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:142
+  bool HasSVE2SHA3 = true;
+  bool HasSVE2BitPerm = true;
+
----------------
rovka wrote:
> Wouldn't it be safer to default to false? Otherwise you'd always have to check hasSVE2() first, which is a bit of a chore.
That does seem logical, this is following the Armv8.4 Crypto extensions defined in this file which can't be seen in this diff because I didn't use a big enough context (about to upload full context). Patch adding those features can be seen here: https://reviews.llvm.org/D48625#change-H8SQOdLbC5nS

@SjoerdMeijer what was the reasoning for setting the default as true for the v8.4 crypto extensions?


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

https://reviews.llvm.org/D61513