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

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added a comment.

> However, the ID_AA64MMFR2_EL1 register was added in v8.2. So I believe we should check as to warn the user if he might get undefined behavior by accessing it when not present.

The encoding of ID_AA64MMFR2_EL1 was defined to read as zero before Armv8.2A, so using it on earlier targets isn't a problem.



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


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


================
Comment at: lib/Target/AArch64/AArch64.td:233
+
+def FeatureCCIDX : SubtargetFeature<
+    "hasCCIDX", "HasCCIDX", "true",
----------------
This feature is in AA64MMFR2_EL1.


================
Comment at: lib/Target/AArch64/AArch64.td:240
+    "Enable v8.3-A Floating-point complex number support",
+    [FeatureNEON]>;
+
----------------
This should also depend on FeatureFullFP16.


================
Comment at: lib/Target/AArch64/AArch64.td:254
+    "hasMPAM", "HasMPAM", "true",
+    "Enable v8.4-A Memory system PArtitioning and Monitoring extension">;
+
----------------
Capitalisation: "PArtitioning"


================
Comment at: lib/Target/AArch64/AArch64.td:261
+def FeatureTRACE : SubtargetFeature<
+    "hasTRACE", "HasTRACE", "true",
+    "Enable v8.4-A Trace extension">;
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64.td:266
+    "hasAM", "HasAM", "true",
+    "Enable v8.4-A Active Monitors extension">;
+
----------------
This should be "Activity", not "Active".


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


================
Comment at: lib/Target/AArch64/AArch64.td:284
+def FeatureWRC : SubtargetFeature<
+    "hasWRC", "HasWRC", "true",
+    "Enable v8.4-A Weaker Release Consistency enhancements">;
----------------
This is an extension over FeatureRCPC which adds immediate-offset versions of the instructions, so maybe FeatureRCPC/"rcpc-offset" would be better?


================
Comment at: lib/Target/AArch64/AArch64.td:321
+// UAO PState
+def FeaturePsUAO : SubtargetFeature< "hasPsUAO", "HasPsUAO", "true",
+    "Enable v8.2 UAO PState", [FeatureID_AA64MMFR2_EL1]>;
----------------
These are v8.2-A features, so they should be further up with the other v8.2A features.


================
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
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64SystemOperands.td:78
 
-let Requires = [{ {AArch64::HasV8_2aOps} }] in {
+let Requires = [{ {AArch64::FeatureATSRW, AArch64::FeatureID_AA64MMFR2_EL1} }] in {
 def : AT<"S1E1RP", 0b000, 0b0111, 0b1001, 0b000>;
----------------
Why does this depend on FeatureID_AA64MMFR2_EL1? This feature is identified by AA64MMFR1_EL1, not AA64MMFR2_EL1.


================
Comment at: lib/Target/AArch64/AArch64SystemOperands.td:351
 // v8.2a "User Access Override" extension-specific PStates
-let Requires = [{ {AArch64::HasV8_2aOps} }] in
+let Requires = [{ {AArch64::FeaturePsUAO, AArch64::FeatureID_AA64MMFR2_EL1} }] in
 def : PState<"UAO",     0b00011>;
----------------
FeaturePsUAO already depends on FeatureID_AA64MMFR2_EL1, so I don't think we need to mention FeatureID_AA64MMFR2_EL1 here.


================
Comment at: lib/Target/AArch64/AArch64SystemOperands.td:1305
 //                              Op0    Op1     CRn     CRm    Op2
-let Requires = [{ {AArch64::HasV8_3aOps} }] in {
+let Requires = [{ {AArch64::FeaturePAN} }] in {
 def : RWSysReg<"APIAKeyLo_EL1", 0b11, 0b000, 0b0010, 0b0001, 0b000>;
----------------
This one should be FeaturePA, FeaturePAN is "Privileged Access Never".


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

https://reviews.llvm.org/D54633