[PATCH] D54633: [NFC][AArch64] Split out backend features

[Diogo N. Sampaio via Phabricator via llvm-commits]

dnsampaio marked 21 inline comments as done.
dnsampaio added inline comments.


================
Comment at: lib/Target/AArch64/AArch64.td:71
+  "Enable ARMv8 Reliability, Availability and Serviceability Extensions",
+   [FeatureID_AA64MMFR2_EL1]>;
 
----------------
olista01 wrote:
> RAS doesn't use ID_AA64MMFR2_EL1.
True.


================
Comment at: lib/Target/AArch64/AArch64.td:92
+def FeaturePAN : SubtargetFeature<
+    "hasPAN", "HasPAN", "true",
+    "Enables ARM v8.1 Privileged Access-Never extension">;
----------------
olista01 wrote:
> None of the existing features have the with "has" in the name (first parameter), so I think we should stick with that convention.
Ok.


================
Comment at: lib/Target/AArch64/AArch64.td:240
+    "Enable v8.3-A Floating-point complex number support",
+    [FeatureNEON]>;
+
----------------
olista01 wrote:
> This should also depend on FeatureFullFP16.
They FP16 instructions are dependent of FullFP16. In AArch64InstrFormats.td, you will see that such instructions depend on ComplNum, NEON and FullFP16.


================
Comment at: lib/Target/AArch64/AArch64.td:261
+def FeatureTRACE : SubtargetFeature<
+    "hasTRACE", "HasTRACE", "true",
+    "Enable v8.4-A Trace extension">;
----------------
olista01 wrote:
> I think this name is too generic, trace has always been a part of the 64-bit architecture, this feature just enables some extensions to it.
Ok, will set to TRACEv8_4.


================
Comment at: lib/Target/AArch64/AArch64.td:273
+def FeatureTLBI : SubtargetFeature<
+    "hasTLBI", "HasTLBI", "true",
+    "Enable v8.4-A TLB range and maintenance Instructions",
----------------
olista01 wrote:
> This name is too generic, maybe "tlbi-range"?
tlb-rmi TLB Range and Management Instructions?


================
Comment at: lib/Target/AArch64/AArch64.td:285
+    "hasWRC", "HasWRC", "true",
+    "Enable v8.4-A Weaker Release Consistency enhancements">;
+
----------------
bryanpkc wrote:
> Should `FeatureWRC` imply `FeatureRCPC`?
Yes it should. Indeed the naming is not the best. Will set it to RCPC_IMMO, as for "RCPC with Immediate Offset".


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:2965
 
-let Predicates = [HasV8_1a] in {
+let Predicates = [HasVH] in {
   // v8.1a "Limited Order Region" extension load-acquire instructions
----------------
olista01 wrote:
> This is the "Limited ordering regions" (ARMv8.1-LOR in the ArmARM) extension, not "Virtualization host extensions" (ARMv8.1-VHE). This will need a separate subtarget feature.
Indeed the feature is there, just got it wrong here. Thanks.


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

https://reviews.llvm.org/D54633