[NFC][AArch64] Document and improve FMV code.

[aaron.ballman]

Generally seems reasonable to me, but I'll give others a chance to comment.

clang/lib/AST/ASTContext.cpp

13459

13529

clang/lib/Basic/Targets/AArch64.cpp

610–612

Good place for llvm::find_if rather than a manual loop?

617

Good place for llvm::find_if rather than a manual loop?

[tmatheson]

LGTM, thanks for making these changes.

llvm/include/llvm/TargetParser/AArch64TargetParser.h

570–571

CPUFeatures has 60 entries, which means the return value here will overflow if we add a few more entries. We should probably have a static_assert(FEAT_MAX <= 64) in the implementation. Or should the CPUFeatures values actually be bitmasks, like ArchExtKind?

[ilinpv]

Rebasing and addressing comments.

[ilinpv]

llvm/include/llvm/TargetParser/AArch64TargetParser.h

570–571

static_assert added. I think changing values to masks could be done separately, it would be good to have if we eventually come to CPUFeatures and ArchExtKind unification.

clang/include/clang/Basic/TargetInfo.h

public:
   }
 
   /// Determine whether this TargetInfo supports the given feature.
   virtual bool isValidFeatureName(StringRef Feature) const {
     return true;
   }
 
   /// Returns true if feature has an impact on target code
-  /// generation and get its dependent options in second argument.
-  virtual bool getFeatureDepOptions(StringRef Feature,
-                                    std::string &Options) const {
+  /// generation.
+  virtual bool doesFeatureAffectCodeGen(StringRef Feature) const {
     return true;
   }
 
+  /// For given feature return dependent ones.
+  virtual StringRef getFeatureDependencies(StringRef Feature) const {
+    return StringRef();
+  }
+
   struct BranchProtectionInfo {
     LangOptions::SignReturnAddressScopeKind SignReturnAddr =
         LangOptions::SignReturnAddressScopeKind::None;
     LangOptions::SignReturnAddressKeyKind SignKey =
         LangOptions::SignReturnAddressKeyKind::AKey;
     bool BranchTargetEnforcement = false;
   };
 

clang/lib/AST/ASTContext.cpp

 std::vector<std::string> ASTContext::filterFunctionTargetVersionAttrs(
     const TargetVersionAttr *TV) const {
   assert(TV != nullptr);
   llvm::SmallVector<StringRef, 8> Feats;
   std::vector<std::string> ResFeats;
   TV->getFeatures(Feats);
   for (auto &Feature : Feats)
     if (Target->validateCpuSupports(Feature.str()))
+      // Use '?' to mark features that came from TargetVersion.
       ResFeats.push_back("?" + Feature.str());
   return ResFeats;
 }
 
 ParsedTargetAttr
 ASTContext::filterFunctionTargetAttrs(const TargetAttr *TD) const {
   assert(TD != nullptr);
   ParsedTargetAttr ParsedAttr = Target->parseTargetAttr(TD->getFeaturesStr());
if (const auto *TD = FD->getAttr<TargetAttr>()) {
     StringRef VersionStr = TC->getFeatureStr(GD.getMultiVersionIndex());
     if (Target->getTriple().isAArch64()) {
       // TargetClones for AArch64
       if (VersionStr != "default") {
         SmallVector<StringRef, 1> VersionFeatures;
         VersionStr.split(VersionFeatures, "+");
         for (auto &VFeature : VersionFeatures) {
           VFeature = VFeature.trim();
+          // Use '?' to mark features that came from AArch64 TargetClones.
           Features.push_back((StringRef{"?"} + VFeature).str());
         }
       }
       Features.insert(Features.begin(),
                       Target->getTargetOpts().FeaturesAsWritten.begin(),
                       Target->getTargetOpts().FeaturesAsWritten.end());
     } else {
       if (VersionStr.startswith("arch="))

clang/lib/Basic/Targets/AArch64.h

void getTargetDefinesARMV94A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 
   std::optional<std::pair<unsigned, unsigned>>
   getVScaleRange(const LangOptions &LangOpts) const override;
-
-  bool getFeatureDepOptions(StringRef Feature,
-                            std::string &Options) const override;
+  bool doesFeatureAffectCodeGen(StringRef Name) const override;
+  StringRef getFeatureDependencies(StringRef Name) const override;
   bool validateCpuSupports(StringRef FeatureStr) const override;
   bool hasFeature(StringRef Feature) const override;
   void setFeatureEnabled(llvm::StringMap<bool> &Features, StringRef Name,
                          bool Enabled) const override;
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override;
   ParsedTargetAttr parseTargetAttr(StringRef Str) const override;
   bool supportsTargetAttributeTune() const override { return true; }

clang/lib/Basic/Targets/AArch64.cpp

unsigned AArch64TargetInfo::multiVersionSortPriority(StringRef Name) const {
   return 0;
 }
 
 unsigned AArch64TargetInfo::multiVersionFeatureCost() const {
   // Take the maximum priority as per feature cost, so more features win.
   return llvm::AArch64::ExtensionInfo::MaxFMVPriority;
 }
 
-bool AArch64TargetInfo::getFeatureDepOptions(StringRef Name,
-                                             std::string &FeatureVec) const {
-  FeatureVec = "";
-  for (const auto &E : llvm::AArch64::Extensions) {
-    if (Name == E.Name) {
-      FeatureVec = E.DependentFeatures;
-      break;
-    }
-  }
-  return FeatureVec != "";
+bool AArch64TargetInfo::doesFeatureAffectCodeGen(StringRef Name) const {
+  auto F = llvm::find_if(llvm::AArch64::Extensions, [&](const auto &E) {
+    return Name == E.Name && !E.DependentFeatures.empty();
+  });
+  return F != std::end(llvm::AArch64::Extensions);
+
+StringRef AArch64TargetInfo::getFeatureDependencies(StringRef Name) const {
+  auto F = llvm::find_if(llvm::AArch64::Extensions,
+                         [&](const auto &E) { return Name == E.Name; });
+  return F != std::end(llvm::AArch64::Extensions) ? F->DependentFeatures
+                                                  : StringRef();
 }
 
 bool AArch64TargetInfo::validateCpuSupports(StringRef FeatureStr) const {
   for (const auto &E : llvm::AArch64::Extensions)
     if (FeatureStr == E.Name)
       return true;
   return false;
 }
if (CpuInfo) {
     llvm::AArch64::getExtensionFeatures(Exts, CPUFeats);
     for (auto F : CPUFeats) {
       assert((F[0] == '+' || F[0] == '-') && "Expected +/- in target feature!");
       UpdatedFeaturesVec.push_back(F.str());
     }
   }
 
   // Process target and dependent features. This is done in two loops collecting
-  // them into UpdatedFeaturesVec: first to add dependent '+'features,
-  // second to add target '+/-'features that can later disable some of
-  // features added on the first loop.
+  // them into UpdatedFeaturesVec: first to add dependent '+'features, second to
+  // add target '+/-'features that can later disable some of features added on
+  // the first loop. Function Multi Versioning features begin with '?'.
   for (const auto &Feature : FeaturesVec)
-    if ((Feature[0] == '?' || Feature[0] == '+')) {
-      std::string Options;
-      if (AArch64TargetInfo::getFeatureDepOptions(Feature.substr(1), Options)) {
-        SmallVector<StringRef, 1> AttrFeatures;
-        StringRef(Options).split(AttrFeatures, ",");
-        for (auto F : AttrFeatures)
-          UpdatedFeaturesVec.push_back(F.str());
-      }
-    }
+    if (((Feature[0] == '?' || Feature[0] == '+')) &&
+        AArch64TargetInfo::doesFeatureAffectCodeGen(Feature.substr(1))) {
+      StringRef DepFeatures =
+          AArch64TargetInfo::getFeatureDependencies(Feature.substr(1));
+      DepFeatures.split(AttrFeatures, ",");
   for (const auto &Feature : FeaturesVec)
     if (Feature[0] != '?') {
       std::string UpdatedFeature = Feature;
       if (Feature[0] == '+') {
         std::optional<llvm::AArch64::ExtensionInfo> Extension =
           llvm::AArch64::parseArchExtension(Feature.substr(1));
         if (Extension)
           UpdatedFeature = Extension->Feature.str();

clang/lib/Sema/SemaDeclAttr.cpp

 bool Sema::checkTargetClonesAttrString(
     SourceLocation LiteralLoc, StringRef Str, const StringLiteral *Literal,
     bool &HasDefault, bool &HasCommas, bool &HasNotDefault,
     SmallVectorImpl<SmallString<64>> &StringsBuffer) {
   enum FirstParam { Unsupported, Duplicate, Unknown };
   enum SecondParam { None, CPU, Tune };
   enum ThirdParam { Target, TargetClones };
   HasCommas = HasCommas || Str.contains(',');
+  const TargetInfo &TInfo = Context.getTargetInfo();
   // Warn on empty at the beginning of a string.
   if (Str.size() == 0)
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
            << Unsupported << None << "" << TargetClones;
 
   std::pair<StringRef, StringRef> Parts = {{}, Str};
   while (!Parts.second.empty()) {
     Parts = Parts.second.split(',');
     StringRef Cur = Parts.first.trim();
-    SourceLocation CurLoc = Literal->getLocationOfByte(
-        Cur.data() - Literal->getString().data(), getSourceManager(),
-        getLangOpts(), Context.getTargetInfo());
+    SourceLocation CurLoc =
+        Literal->getLocationOfByte(Cur.data() - Literal->getString().data(),
+                                   getSourceManager(), getLangOpts(), TInfo);
 
     bool DefaultIsDupe = false;
     bool HasCodeGenImpact = false;
     if (Cur.empty())
       return Diag(CurLoc, diag::warn_unsupported_target_attribute)
              << Unsupported << None << "" << TargetClones;
 
-    if (Context.getTargetInfo().getTriple().isAArch64()) {
+    if (TInfo.getTriple().isAArch64()) {
       // AArch64 target clones specific
       if (Cur == "default") {
         DefaultIsDupe = HasDefault;
         HasDefault = true;
         if (llvm::is_contained(StringsBuffer, Cur) || DefaultIsDupe)
           Diag(CurLoc, diag::warn_target_clone_duplicate_options);
         else
           StringsBuffer.push_back(Cur);
       } else {
         std::pair<StringRef, StringRef> CurParts = {{}, Cur};
         llvm::SmallVector<StringRef, 8> CurFeatures;
         while (!CurParts.second.empty()) {
           CurParts = CurParts.second.split('+');
           StringRef CurFeature = CurParts.first.trim();
-          if (!Context.getTargetInfo().validateCpuSupports(CurFeature)) {
+          if (!TInfo.validateCpuSupports(CurFeature)) {
             Diag(CurLoc, diag::warn_unsupported_target_attribute)
                 << Unsupported << None << CurFeature << TargetClones;
             continue;
           }
-          std::string Options;
-          if (Context.getTargetInfo().getFeatureDepOptions(CurFeature, Options))
+          if (TInfo.doesFeatureAffectCodeGen(CurFeature))
             HasCodeGenImpact = true;
           CurFeatures.push_back(CurFeature);
         }
         // Canonize TargetClones Attributes
         llvm::sort(CurFeatures);
         SmallString<64> Res;
         for (auto &CurFeat : CurFeatures) {
           if (!Res.equals(""))

llvm/include/llvm/TargetParser/AArch64TargetParser.h

 #include <array>
 #include <vector>
 
 namespace llvm {
 
 class Triple;
 
 namespace AArch64 {
+// Function Multi Versioning CPU features. They must be kept in sync with
+// compiler-rt enum CPUFeatures in lib/builtins/cpu_model.c with FEAT_MAX as
+// sentinel.
 enum CPUFeatures {
   FEAT_RNG,
   FEAT_FLAGM,
   FEAT_FLAGM2,
   FEAT_FP16FML,
   FEAT_DOTPROD,
   FEAT_SM4,
   FEAT_RDM,
enum CPUFeatures {
   FEAT_LS64_ACCDATA,
   FEAT_WFXT,
   FEAT_SME_F64,
   FEAT_SME_I64,
   FEAT_SME2,
   FEAT_MAX
 };
 
+static_assert(FEAT_MAX <= 64,
+              "CPUFeatures enum must not have more than 64 entries");
+
 // Arch extension modifiers for CPUs. These are labelled with their Arm ARM
 // feature name (though the canonical reference for those is AArch64.td)
 // clang-format off
 enum ArchExtKind : uint64_t {
   AEK_NONE =        1,
   AEK_CRC =         1 << 1,  // FEAT_CRC32
   AEK_CRYPTO =      1 << 2,
   AEK_FP =          1 << 3,  // FEAT_FP

 };
 // clang-format on
 
 // Represents an extension that can be enabled with -march=<arch>+<extension>.
 // Typically these correspond to Arm Architecture extensions, unlike
 // SubtargetFeature which may represent either an actual extension or some
 // internal LLVM property.
 struct ExtensionInfo {
-  StringRef Name;       // Human readable name, e.g. "profile".
-  ArchExtKind ID;       // Corresponding to the ArchExtKind, this extensions
-                        // representation in the bitfield.
-  StringRef Feature;    // -mattr enable string, e.g. "+spe"
-  StringRef NegFeature; // -mattr disable string, e.g. "-spe"
-
-  // FIXME These were added by D127812 FMV support and need documenting:
-  CPUFeatures CPUFeature; // Bitfield value set in __aarch64_cpu_features
-  StringRef DependentFeatures;
-  unsigned FmvPriority;
-  static constexpr unsigned MaxFMVPriority = 1000;
+  StringRef Name;              // Human readable name, e.g. "profile".
+  ArchExtKind ID;              // Corresponding to the ArchExtKind, this
+                               // extensions representation in the bitfield.
+  StringRef Feature;           // -mattr enable string, e.g. "+spe"
+  StringRef NegFeature;        // -mattr disable string, e.g. "-spe"
+  CPUFeatures CPUFeature;      // Function Multi Versioning (FMV) bitfield value
+                               // set in __aarch64_cpu_features
+  StringRef DependentFeatures; // FMV enabled features string,
+                               // e.g. "+dotprod,+fp-armv8,+neon"
+  unsigned FmvPriority;        // FMV feature priority
+  static constexpr unsigned MaxFMVPriority =
+      1000; // Maximum priority for FMV feature
 };
 
 // clang-format off
 inline constexpr ExtensionInfo Extensions[] = {
     {"aes", AArch64::AEK_AES, "+aes", "-aes", FEAT_AES, "+fp-armv8,+neon", 150},
     {"b16b16", AArch64::AEK_B16B16, "+b16b16", "-b16b16", FEAT_MAX, "", 0},
     {"bf16", AArch64::AEK_BF16, "+bf16", "-bf16", FEAT_BF16, "+bf16", 280},
     {"brbe", AArch64::AEK_BRBE, "+brbe", "-brbe", FEAT_MAX, "", 0},

 std::optional<ArchInfo> parseArch(StringRef Arch);
 std::optional<ExtensionInfo> parseArchExtension(StringRef Extension);
 // Given the name of a CPU or alias, return the correponding CpuInfo.
 std::optional<CpuInfo> parseCpu(StringRef Name);
 // Used by target parser tests
 void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values);
 
 bool isX18ReservedByDefault(const Triple &TT);
+
+// For given feature names, return a bitmask corresponding to the entries of
+// AArch64::CPUFeatures. The values in CPUFeatures are not bitmasks
+// themselves, they are sequential (0, 1, 2, 3, ...).
 uint64_t getCpuSupportsMask(ArrayRef<StringRef> FeatureStrs);
 
 } // namespace AArch64
 } // namespace llvm
 
 #endif