[clang] [llvm] [AArch64] Merge duplicate extension information. (PR #92319)

Tomas Matheson via llvm-commits llvm-commits at lists.llvm.org
Fri May 17 13:37:55 PDT 2024


================
@@ -56,43 +52,64 @@ class Extension<
 
     // The FMV priority
     int FMVPriority = _FMVPriority;
+
+    // Indicates if the extension is available on the command line.
+    string IsFMVOnly = _IsFMVOnly;
 }
 
 // Some extensions are available for FMV but can not be controlled via the
-// command line. These entries:
-//  - are SubtargetFeatures, so they have (unused) FieldNames on the subtarget
-//    e.g. HasFMVOnlyFEAT_XYZ
-//  - have incorrect (empty) Implies fields, because the code that handles FMV
-//    ignores these dependencies and looks only at FMVDependencies.
-//  - have no description.
-// 
-// In the generated data structures for extensions (ExtensionInfo), AEK_NONE is
-// used to indicate that a feature is FMV only. Therefore ArchExtKindSpelling is
-// manually overridden here.
-class FMVOnlyExtension<string FMVBit, string Name, string Deps, int Priority>
-  : Extension<Name, "FMVOnly"#FMVBit, "", [], FMVBit, Deps, Priority> {
-    let ArchExtKindSpelling = "AEK_NONE"; // AEK_NONE indicates FMV-only feature
-}
+// command line, neither have a TargetFeatureName. Since they have no effect
+// on their own, their description is left empty. However they can have side
+// effects by implying other Subtarget Features. These extensions are used
+// in FMV for detection purposes.
+
+let MArchName = "dgh" in
+def : Extension<"", "DGH", "", [], "FEAT_DGH", "", 260, "true">;
----------------
tmatheson-arm wrote:

I think this is a regression over using the `FMVOnlyExtension` class:
- These extensions are _not_ available via `-march`, but they require writing `let MArchName = "..."` while leaving their actual `Name` empty. That seems the wrong way around?
- The `FMVOnlyExtension` type makes it obvious what it is, whereas the boolean parameter makes it obscure.
- There are now subtarget fields like `HasDGH` which are not obviously FMV-only. Previously they were called `HasFMVOnlyDGH` etc.
- Removing the type and relying on convention leaves it open to abusing the abstraction, which as we have seen will happen very quickly.

This could be done while keeping the type:
```cpp
class FMVOnlyExtension<string FMVBit, string Name, string Deps, int Priority>
  : Extension<"", "FMVOnly"#FMVBit, "", [], FMVBit, Deps, Priority> {
    let IsFMVOnly = 1;
    // comment explaining why MArchName is overridden despite not working for -march
    let MArchName = Name;
}
```

https://github.com/llvm/llvm-project/pull/92319


More information about the llvm-commits mailing list