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

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

dnsampaio added a comment.

Hi @olista01 and @bryanpkc. Thanks for the reviews.

In https://reviews.llvm.org/D54633#1306653, @olista01 wrote:

> Have you discussed these feature names with the GCC devs? I know we can change the user-facing names used by clang in in TargetParser, but it's easier for us if they match.


Indeed I did not. As you said, they are not visible to the user. I'll check with the gcc devs how they have these features split and named though.

> For the feature registers (ISAR6 and MMFR2), maybe we should just make these registers always available? These registers are all defined to read as all-zeroes in older architectures, which means no features described by that register are implemented. If we continue like this then we will have to add an extra feature to pretty much every new architectural feature for the ID register.

Indeed it seems the case for the ID_ISAR6_EL1 exist, so I'll just remove the constraints over it.
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.



================
Comment at: lib/Target/AArch64/AArch64.td:36
+// UAO PState
+def FeaturePsUAO : SubtargetFeature<
+    "hasPsUAO", "HasPsUAO", "true",
----------------
olista01 wrote:
> Why is this up here, and not with the other v8.2A features?
I have not seen any pstates and registers Features before, so I created a region apart. No technical problem with it. I'll move UAO to the other features.


================
Comment at: lib/Target/AArch64/AArch64.td:99
+  "Enable FP16 FML instructions",
+  [FeatureFullFP16, FeatureID_AA64MMFR2_EL1]>;
 
----------------
bryanpkc wrote:
> Shouldn't this feature imply `FeatureID_ISAR6_EL1` instead of `FeatureID_AA64MMFR2_EL1`?
Indeed, ID_ISAR6_EL1 is used in arm32, not arm64. In arm64 it is identified using ID_AA64ISAR0_EL1, that has no constraints. So I can just remove this implication.


================
Comment at: lib/Target/AArch64/AArch64.td:236
     "dotprod", "HasDotProd", "true",
-    "Enable dot product support">;
+    "Enable dot product support", [FeatureID_AA64MMFR2_EL1]>;
+
----------------
bryanpkc wrote:
> Shouldn't this feature imply `FeatureID_ISAR6_EL1` instead of `FeatureID_AA64MMFR2_EL1`?
> 
> 
Yes, and same as the above, the implication is not required.


================
Comment at: lib/Target/AArch64/AArch64.td:245
+    "Enable v8.3-A Java Script FP conversion enchancement",
+    [FeatureFPARMv8, FeatureID_AA64MMFR2_EL1]>;
+
----------------
bryanpkc wrote:
> Shouldn't this feature imply `FeatureID_ISAR6_EL1` instead of `FeatureID_AA64MMFR2_EL1`?
> 
> IMO, the description should say `Javascript` or `JavaScript` to make it easier to grep.
You are right with the naming. The space in `Java Script` was not intentional.
Nice catch again about the registers. Indeed for arm64 it is ID_AA64ISAR0_EL1.FHM using bits [51:48]. So I can just remove this implication.


================
Comment at: lib/Target/AArch64/AArch64.td:289
+    "Enable v8.4-A TLB range and maintenance Instructions",
+    [FeatureID_AA64MMFR2_EL1]>;
+
----------------
bryanpkc wrote:
> The architecture reference manual states that the ARMv8.4-TLBI feature is identified by `ID_AA64ISAR0_EL1.TLB`, not `ID_AA64MMFR2_EL1`. Is this discrepancy intentional?
Indeed the TLB instructions features are identified by ID_AA64ISAR0_EL1[59:56]. 
However, the use for TTL field in address operations is identified by ID_AA64MMFR2_EL1[51:48] field TTL. So this feature holds true. Even if the processor does not support it, it requires the register to tell it does not support it.


https://reviews.llvm.org/D54633