[clang] [llvm] [NFC][clang][FMV][TargetInfo] Refactor API for FMV feature priority. (PR #116257)

Alexandros Lamprineas via cfe-commits cfe-commits at lists.llvm.org
Wed Nov 27 07:08:22 PST 2024


https://github.com/labrinea updated https://github.com/llvm/llvm-project/pull/116257

>From eb6fea771b0824fef979e5eef26718ecbc8c7f56 Mon Sep 17 00:00:00 2001
From: Alexandros Lamprineas <alexandros.lamprineas at arm.com>
Date: Thu, 14 Nov 2024 16:07:32 +0000
Subject: [PATCH 1/6] [NFC][clang][FMV][TargetInfo] Refactor API for FMV
 feature priority.

Currently we have code with target hooks in CodeGenModule shared
between X86 and AArch64 for sorting MultiVersionResolverOptions.
Those are used when generating IFunc resolvers for FMV. The RISCV
target has different criteria for sorting, therefore it repeats
sorting after calling CodeGenFunction::EmitMultiVersionResolver.

I am moving the FMV priority logic in TargetInfo, so that it can
be implemented by the TargetParser which then makes it possible
to query it from llvm. Here is an example why this is handy:
https://github.com/llvm/llvm-project/pull/87939
---
 clang/include/clang/Basic/TargetInfo.h        |  6 +--
 clang/lib/Basic/Targets/AArch64.cpp           | 14 +------
 clang/lib/Basic/Targets/AArch64.h             |  3 +-
 clang/lib/Basic/Targets/RISCV.cpp             | 15 ++++++++
 clang/lib/Basic/Targets/RISCV.h               |  1 +
 clang/lib/Basic/Targets/X86.cpp               | 35 ++++++++++-------
 clang/lib/Basic/Targets/X86.h                 |  2 +-
 clang/lib/CodeGen/ABIInfo.cpp                 |  4 +-
 clang/lib/CodeGen/CodeGenFunction.cpp         | 38 +++----------------
 clang/lib/CodeGen/CodeGenModule.cpp           | 23 +++--------
 .../llvm/TargetParser/AArch64TargetParser.h   |  3 ++
 llvm/lib/TargetParser/AArch64TargetParser.cpp | 13 +++++++
 12 files changed, 72 insertions(+), 85 deletions(-)

diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 25eda907d20a7b..c27f7eb591f43d 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -1514,14 +1514,10 @@ class TargetInfo : public TransferrableTargetInfo,
 
   // Return the target-specific priority for features/cpus/vendors so
   // that they can be properly sorted for checking.
-  virtual unsigned multiVersionSortPriority(StringRef Name) const {
+  virtual unsigned getFMVPriority(ArrayRef<StringRef> Features) const {
     return 0;
   }
 
-  // Return the target-specific cost for feature
-  // that taken into account in priority sorting.
-  virtual unsigned multiVersionFeatureCost() const { return 0; }
-
   // Validate the contents of the __builtin_cpu_is(const char*)
   // argument.
   virtual bool validateCpuIs(StringRef Name) const { return false; }
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index eb8a3ada034482..4efc1841c836d0 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -705,18 +705,8 @@ AArch64TargetInfo::getVScaleRange(const LangOptions &LangOpts) const {
   return std::nullopt;
 }
 
-unsigned AArch64TargetInfo::multiVersionSortPriority(StringRef Name) const {
-  if (Name == "default")
-    return 0;
-  if (auto Ext = llvm::AArch64::parseFMVExtension(Name))
-    return Ext->Priority;
-  return 0;
-}
-
-unsigned AArch64TargetInfo::multiVersionFeatureCost() const {
-  // Take the maximum priority as per feature cost, so more features win.
-  constexpr unsigned MaxFMVPriority = 1000;
-  return MaxFMVPriority;
+unsigned AArch64TargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
+  return llvm::AArch64::getFMVPriority(Features);
 }
 
 bool AArch64TargetInfo::doesFeatureAffectCodeGen(StringRef Name) const {
diff --git a/clang/lib/Basic/Targets/AArch64.h b/clang/lib/Basic/Targets/AArch64.h
index 4c25bdb5bb16df..68a8b1ebad8cde 100644
--- a/clang/lib/Basic/Targets/AArch64.h
+++ b/clang/lib/Basic/Targets/AArch64.h
@@ -137,8 +137,7 @@ class LLVM_LIBRARY_VISIBILITY AArch64TargetInfo : public TargetInfo {
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
   bool setCPU(const std::string &Name) override;
 
-  unsigned multiVersionSortPriority(StringRef Name) const override;
-  unsigned multiVersionFeatureCost() const override;
+  unsigned getFMVPriority(ArrayRef<StringRef> Features) const override;
 
   bool useFP16ConversionIntrinsics() const override {
     return false;
diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index eaaba7642bd7b2..d5312bb171af6c 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -476,6 +476,21 @@ ParsedTargetAttr RISCVTargetInfo::parseTargetAttr(StringRef Features) const {
   return Ret;
 }
 
+unsigned RISCVTargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
+  SmallVector<StringRef, 8> Attrs;
+  Features[0].split(Attrs, ';');
+  // Default Priority is zero.
+  unsigned Priority = 0;
+  for (auto Attr : Attrs) {
+    if (Attr.consume_front("priority=")) {
+      unsigned Result;
+      if (!Attr.getAsInteger(0, Result))
+        Priority = Result;
+    }
+  }
+  return Priority;
+}
+
 TargetInfo::CallingConvCheckResult
 RISCVTargetInfo::checkCallingConvention(CallingConv CC) const {
   switch (CC) {
diff --git a/clang/lib/Basic/Targets/RISCV.h b/clang/lib/Basic/Targets/RISCV.h
index 3b418585ab4a39..0a6caeb54bbd5d 100644
--- a/clang/lib/Basic/Targets/RISCV.h
+++ b/clang/lib/Basic/Targets/RISCV.h
@@ -122,6 +122,7 @@ class RISCVTargetInfo : public TargetInfo {
   void fillValidTuneCPUList(SmallVectorImpl<StringRef> &Values) const override;
   bool supportsTargetAttributeTune() const override { return true; }
   ParsedTargetAttr parseTargetAttr(StringRef Str) const override;
+  unsigned getFMVPriority(ArrayRef<StringRef> Features) const override;
 
   std::pair<unsigned, unsigned> hardwareInterferenceSizes() const override {
     return std::make_pair(32, 32);
diff --git a/clang/lib/Basic/Targets/X86.cpp b/clang/lib/Basic/Targets/X86.cpp
index 086b4415412e67..ce5ccd0447b437 100644
--- a/clang/lib/Basic/Targets/X86.cpp
+++ b/clang/lib/Basic/Targets/X86.cpp
@@ -1363,19 +1363,28 @@ static llvm::X86::ProcessorFeatures getFeature(StringRef Name) {
   // correct, so it asserts if the value is out of range.
 }
 
-unsigned X86TargetInfo::multiVersionSortPriority(StringRef Name) const {
-  // Valid CPUs have a 'key feature' that compares just better than its key
-  // feature.
-  using namespace llvm::X86;
-  CPUKind Kind = parseArchX86(Name);
-  if (Kind != CK_None) {
-    ProcessorFeatures KeyFeature = getKeyFeature(Kind);
-    return (getFeaturePriority(KeyFeature) << 1) + 1;
-  }
-
-  // Now we know we have a feature, so get its priority and shift it a few so
-  // that we have sufficient room for the CPUs (above).
-  return getFeaturePriority(getFeature(Name)) << 1;
+unsigned X86TargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
+  auto getPriority = [this](StringRef Feature) -> unsigned {
+    if (Feature.empty())
+      return 0;
+
+    // Valid CPUs have a 'key feature' that compares just better than its key
+    // feature.
+    using namespace llvm::X86;
+    CPUKind Kind = parseArchX86(Feature);
+    if (Kind != CK_None) {
+      ProcessorFeatures KeyFeature = getKeyFeature(Kind);
+      return (getFeaturePriority(KeyFeature) << 1) + 1;
+    }
+    // Now we know we have a feature, so get its priority and shift it a few so
+    // that we have sufficient room for the CPUs (above).
+    return getFeaturePriority(getFeature(Feature)) << 1;
+  };
+
+  unsigned Priority = 0;
+  for (StringRef Feature : Features)
+    Priority = std::max(Priority, getPriority(Feature));
+  return Priority;
 }
 
 bool X86TargetInfo::validateCPUSpecificCPUDispatch(StringRef Name) const {
diff --git a/clang/lib/Basic/Targets/X86.h b/clang/lib/Basic/Targets/X86.h
index 06a7eed8177cb2..3ed36c8fa724b5 100644
--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -384,7 +384,7 @@ class LLVM_LIBRARY_VISIBILITY X86TargetInfo : public TargetInfo {
     return CPU != llvm::X86::CK_None;
   }
 
-  unsigned multiVersionSortPriority(StringRef Name) const override;
+  unsigned getFMVPriority(ArrayRef<StringRef> Features) const override;
 
   bool setFPMath(StringRef Name) override;
 
diff --git a/clang/lib/CodeGen/ABIInfo.cpp b/clang/lib/CodeGen/ABIInfo.cpp
index edd7146dc1ac76..8e76cf15b642c6 100644
--- a/clang/lib/CodeGen/ABIInfo.cpp
+++ b/clang/lib/CodeGen/ABIInfo.cpp
@@ -218,8 +218,8 @@ void ABIInfo::appendAttributeMangling(StringRef AttrStr,
     // only have "+" prefixes here.
     assert(LHS.starts_with("+") && RHS.starts_with("+") &&
            "Features should always have a prefix.");
-    return TI.multiVersionSortPriority(LHS.substr(1)) >
-           TI.multiVersionSortPriority(RHS.substr(1));
+    return TI.getFMVPriority({LHS.substr(1)}) >
+           TI.getFMVPriority({RHS.substr(1)});
   });
 
   bool IsFirst = true;
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 6ead45793742d6..7ac60e8dc0508e 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2904,24 +2904,6 @@ void CodeGenFunction::EmitMultiVersionResolver(
   }
 }
 
-static unsigned getPriorityFromAttrString(StringRef AttrStr) {
-  SmallVector<StringRef, 8> Attrs;
-
-  AttrStr.split(Attrs, ';');
-
-  // Default Priority is zero.
-  unsigned Priority = 0;
-  for (auto Attr : Attrs) {
-    if (Attr.consume_front("priority=")) {
-      unsigned Result;
-      if (!Attr.getAsInteger(0, Result))
-        Priority = Result;
-    }
-  }
-
-  return Priority;
-}
-
 void CodeGenFunction::EmitRISCVMultiVersionResolver(
     llvm::Function *Resolver, ArrayRef<MultiVersionResolverOption> Options) {
 
@@ -2939,20 +2921,10 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
   bool HasDefault = false;
   unsigned DefaultIndex = 0;
 
-  SmallVector<CodeGenFunction::MultiVersionResolverOption, 10> CurrOptions(
-      Options);
-
-  llvm::stable_sort(
-      CurrOptions, [](const CodeGenFunction::MultiVersionResolverOption &LHS,
-                      const CodeGenFunction::MultiVersionResolverOption &RHS) {
-        return getPriorityFromAttrString(LHS.Conditions.Features[0]) >
-               getPriorityFromAttrString(RHS.Conditions.Features[0]);
-      });
-
   // Check the each candidate function.
-  for (unsigned Index = 0; Index < CurrOptions.size(); Index++) {
+  for (unsigned Index = 0; Index < Options.size(); Index++) {
 
-    if (CurrOptions[Index].Conditions.Features[0].starts_with("default")) {
+    if (Options[Index].Conditions.Features[0].starts_with("default")) {
       HasDefault = true;
       DefaultIndex = Index;
       continue;
@@ -2963,7 +2935,7 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
     std::vector<std::string> TargetAttrFeats =
         getContext()
             .getTargetInfo()
-            .parseTargetAttr(CurrOptions[Index].Conditions.Features[0])
+            .parseTargetAttr(Options[Index].Conditions.Features[0])
             .Features;
 
     if (TargetAttrFeats.empty())
@@ -3004,7 +2976,7 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
     llvm::BasicBlock *RetBlock = createBasicBlock("resolver_return", Resolver);
     CGBuilderTy RetBuilder(*this, RetBlock);
     CreateMultiVersionResolverReturn(
-        CGM, Resolver, RetBuilder, CurrOptions[Index].Function, SupportsIFunc);
+        CGM, Resolver, RetBuilder, Options[Index].Function, SupportsIFunc);
     llvm::BasicBlock *ElseBlock = createBasicBlock("resolver_else", Resolver);
 
     Builder.SetInsertPoint(CurBlock);
@@ -3017,7 +2989,7 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
   if (HasDefault) {
     Builder.SetInsertPoint(CurBlock);
     CreateMultiVersionResolverReturn(CGM, Resolver, Builder,
-                                     CurrOptions[DefaultIndex].Function,
+                                     Options[DefaultIndex].Function,
                                      SupportsIFunc);
     return;
   }
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index ba376f9ecfacde..fa569810da6296 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4216,22 +4216,11 @@ static void ReplaceUsesOfNonProtoTypeWithRealFunction(llvm::GlobalValue *Old,
                                                       llvm::Function *NewFn);
 
 static unsigned
-TargetMVPriority(const TargetInfo &TI,
-                 const CodeGenFunction::MultiVersionResolverOption &RO) {
-  unsigned Priority = 0;
-  unsigned NumFeatures = 0;
-  for (StringRef Feat : RO.Conditions.Features) {
-    Priority = std::max(Priority, TI.multiVersionSortPriority(Feat));
-    NumFeatures++;
-  }
-
-  if (!RO.Conditions.Architecture.empty())
-    Priority = std::max(
-        Priority, TI.multiVersionSortPriority(RO.Conditions.Architecture));
-
-  Priority += TI.multiVersionFeatureCost() * NumFeatures;
-
-  return Priority;
+getFMVPriority(const TargetInfo &TI,
+               const CodeGenFunction::MultiVersionResolverOption &RO) {
+  llvm::SmallVector<StringRef, 8> Features{RO.Conditions.Features};
+  Features.push_back(RO.Conditions.Architecture);
+  return TI.getFMVPriority(Features);
 }
 
 // Multiversion functions should be at most 'WeakODRLinkage' so that a different
@@ -4362,7 +4351,7 @@ void CodeGenModule::emitMultiVersionFunctions() {
     llvm::stable_sort(
         Options, [&TI](const CodeGenFunction::MultiVersionResolverOption &LHS,
                        const CodeGenFunction::MultiVersionResolverOption &RHS) {
-          return TargetMVPriority(TI, LHS) > TargetMVPriority(TI, RHS);
+          return getFMVPriority(TI, LHS) > getFMVPriority(TI, RHS);
         });
     CodeGenFunction CGF(*this);
     CGF.EmitMultiVersionResolver(ResolverFunc, Options);
diff --git a/llvm/include/llvm/TargetParser/AArch64TargetParser.h b/llvm/include/llvm/TargetParser/AArch64TargetParser.h
index d7b1ba511f95d3..1311329821828f 100644
--- a/llvm/include/llvm/TargetParser/AArch64TargetParser.h
+++ b/llvm/include/llvm/TargetParser/AArch64TargetParser.h
@@ -268,6 +268,9 @@ void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values);
 
 bool isX18ReservedByDefault(const Triple &TT);
 
+// Return the priority for a given set of FMV features.
+unsigned getFMVPriority(ArrayRef<StringRef> Features);
+
 // 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, ...).
diff --git a/llvm/lib/TargetParser/AArch64TargetParser.cpp b/llvm/lib/TargetParser/AArch64TargetParser.cpp
index 45ecc4f24c2afd..fe5ab0fabefa6e 100644
--- a/llvm/lib/TargetParser/AArch64TargetParser.cpp
+++ b/llvm/lib/TargetParser/AArch64TargetParser.cpp
@@ -48,6 +48,19 @@ std::optional<AArch64::ArchInfo> AArch64::ArchInfo::findBySubArch(StringRef SubA
   return {};
 }
 
+unsigned AArch64::getFMVPriority(ArrayRef<StringRef> Features) {
+  constexpr unsigned MaxFMVPriority = 1000;
+  unsigned Priority = 0;
+  unsigned NumFeatures = 0;
+  for (StringRef Feature : Features) {
+    if (auto Ext = parseFMVExtension(Feature)) {
+      Priority = std::max(Priority, Ext->Priority);
+      NumFeatures++;
+    }
+  }
+  return Priority + MaxFMVPriority * NumFeatures;
+}
+
 uint64_t AArch64::getCpuSupportsMask(ArrayRef<StringRef> FeatureStrs) {
   uint64_t FeaturesMask = 0;
   for (const StringRef &FeatureStr : FeatureStrs) {

>From 1ad27f7a240a7f214b8bcee763eb40c54574dbcb Mon Sep 17 00:00:00 2001
From: Alexandros Lamprineas <alexandros.lamprineas at arm.com>
Date: Fri, 15 Nov 2024 10:30:50 +0000
Subject: [PATCH 2/6] Update CodeGenFunction.cpp

---
 clang/lib/CodeGen/CodeGenFunction.cpp | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 7ac60e8dc0508e..a118a16f4f0235 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2975,8 +2975,8 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
 
     llvm::BasicBlock *RetBlock = createBasicBlock("resolver_return", Resolver);
     CGBuilderTy RetBuilder(*this, RetBlock);
-    CreateMultiVersionResolverReturn(
-        CGM, Resolver, RetBuilder, Options[Index].Function, SupportsIFunc);
+    CreateMultiVersionResolverReturn(CGM, Resolver, RetBuilder,
+                                     Options[Index].Function, SupportsIFunc);
     llvm::BasicBlock *ElseBlock = createBasicBlock("resolver_else", Resolver);
 
     Builder.SetInsertPoint(CurBlock);
@@ -2988,9 +2988,8 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
   // Finally, emit the default one.
   if (HasDefault) {
     Builder.SetInsertPoint(CurBlock);
-    CreateMultiVersionResolverReturn(CGM, Resolver, Builder,
-                                     Options[DefaultIndex].Function,
-                                     SupportsIFunc);
+    CreateMultiVersionResolverReturn(
+        CGM, Resolver, Builder, Options[DefaultIndex].Function, SupportsIFunc);
     return;
   }
 

>From a3a9f46737410aff34675c033dbbc80f9ebb13dd Mon Sep 17 00:00:00 2001
From: Alexandros Lamprineas <alexandros.lamprineas at arm.com>
Date: Mon, 18 Nov 2024 10:15:15 +0000
Subject: [PATCH 3/6] Update X86.cpp

moved condition outside of lambda
---
 clang/lib/Basic/Targets/X86.cpp | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/clang/lib/Basic/Targets/X86.cpp b/clang/lib/Basic/Targets/X86.cpp
index ce5ccd0447b437..0b72c2d227aa8d 100644
--- a/clang/lib/Basic/Targets/X86.cpp
+++ b/clang/lib/Basic/Targets/X86.cpp
@@ -1365,9 +1365,6 @@ static llvm::X86::ProcessorFeatures getFeature(StringRef Name) {
 
 unsigned X86TargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
   auto getPriority = [this](StringRef Feature) -> unsigned {
-    if (Feature.empty())
-      return 0;
-
     // Valid CPUs have a 'key feature' that compares just better than its key
     // feature.
     using namespace llvm::X86;
@@ -1383,7 +1380,8 @@ unsigned X86TargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
 
   unsigned Priority = 0;
   for (StringRef Feature : Features)
-    Priority = std::max(Priority, getPriority(Feature));
+    if (!Feature.empty())
+      Priority = std::max(Priority, getPriority(Feature));
   return Priority;
 }
 

>From c147f0cd439812114322e4edb72795d848af3d6d Mon Sep 17 00:00:00 2001
From: Alexandros Lamprineas <alexandros.lamprineas at arm.com>
Date: Tue, 19 Nov 2024 13:55:36 +0000
Subject: [PATCH 4/6] Changes from last revision:

* Parse riscv fmv attributes before passing to resolver options
* Simplify resolver options struct
---
 clang/include/clang/Basic/Attr.td     | 71 +++++++++++++++++++--------
 clang/lib/Basic/Targets/RISCV.cpp     | 27 +++++-----
 clang/lib/CodeGen/CodeGenFunction.cpp | 65 +++++++++++-------------
 clang/lib/CodeGen/CodeGenFunction.h   | 39 ++++++---------
 clang/lib/CodeGen/CodeGenModule.cpp   | 68 +++++++++++--------------
 5 files changed, 137 insertions(+), 133 deletions(-)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 6035a563d5fce7..4dccce7cd9e0f7 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3260,19 +3260,16 @@ def Target : InheritableAttr {
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [TargetDocs];
   let AdditionalMembers = [{
-    StringRef getArchitecture() const {
+    std::optional<StringRef> getArchitecture() const {
       StringRef Features = getFeaturesStr();
-      if (Features == "default") return {};
-
-      SmallVector<StringRef, 1> AttrFeatures;
+      SmallVector<StringRef, 4> AttrFeatures;
       Features.split(AttrFeatures, ",");
-
-      for (auto &Feature : AttrFeatures) {
+      for (StringRef Feature : AttrFeatures) {
         Feature = Feature.trim();
         if (Feature.starts_with("arch="))
           return Feature.drop_front(sizeof("arch=") - 1);
       }
-      return "";
+      return std::nullopt;
     }
 
     // Gets the list of features as simple string-refs with no +/- or 'no-'.
@@ -3304,17 +3301,17 @@ def TargetVersion : InheritableAttr {
   let Documentation = [TargetVersionDocs];
   let AdditionalMembers = [{
     StringRef getName() const { return getNamesStr().trim(); }
-    bool isDefaultVersion() const {
-      return getName() == "default";
-    }
-    void getFeatures(llvm::SmallVectorImpl<StringRef> &Out) const {
-      if (isDefaultVersion()) return;
-      StringRef Features = getName();
 
-      SmallVector<StringRef, 8> AttrFeatures;
-      Features.split(AttrFeatures, "+");
+    bool isDefaultVersion() const { return getName() == "default"; }
 
-      for (auto &Feature : AttrFeatures) {
+    void getFeatures(llvm::SmallVectorImpl<StringRef> &Out,
+                     char Delim = '+') const {
+      if (isDefaultVersion())
+        return;
+      StringRef Features = getName();
+      SmallVector<StringRef, 4> AttrFeatures;
+      Features.split(AttrFeatures, Delim);
+      for (StringRef Feature : AttrFeatures) {
         Feature = Feature.trim();
         Out.push_back(Feature);
       }
@@ -3331,20 +3328,52 @@ def TargetClones : InheritableAttr {
     StringRef getFeatureStr(unsigned Index) const {
       return *(featuresStrs_begin() + Index);
     }
+
     bool isDefaultVersion(unsigned Index) const {
       return getFeatureStr(Index) == "default";
     }
+
     void getFeatures(llvm::SmallVectorImpl<StringRef> &Out,
-                     unsigned Index) const {
-      if (isDefaultVersion(Index)) return;
+                     unsigned Index, char Delim = '+') const {
+      if (isDefaultVersion(Index))
+        return;
       StringRef Features = getFeatureStr(Index);
-      SmallVector<StringRef, 8> AttrFeatures;
-      Features.split(AttrFeatures, "+");
-      for (auto &Feature : AttrFeatures) {
+      SmallVector<StringRef, 4> AttrFeatures;
+      Features.split(AttrFeatures, Delim);
+      for (StringRef Feature : AttrFeatures) {
         Feature = Feature.trim();
         Out.push_back(Feature);
       }
     }
+
+    // x86 only.
+    std::optional<StringRef> getArchitecture(unsigned Index) const {
+      StringRef Features = getFeatureStr(Index);
+      SmallVector<StringRef, 4> AttrFeatures;
+      Features.split(AttrFeatures, ',');
+      for (StringRef Feature : AttrFeatures) {
+        Feature = Feature.trim();
+        if (Feature.starts_with("arch="))
+          return Feature.drop_front(sizeof("arch=") - 1);
+      }
+      return std::nullopt;
+    }
+
+    // x86 only.
+    void getAddedFeatures(llvm::SmallVectorImpl<StringRef> &Out,
+                          unsigned Index) const {
+      if (isDefaultVersion(Index))
+        return;
+      StringRef Features = getFeatureStr(Index);
+      SmallVector<StringRef, 4> AttrFeatures;
+      Features.split(AttrFeatures, ",");
+      for (StringRef Feature : AttrFeatures) {
+        Feature = Feature.trim();
+        if (!Feature.starts_with("arch="))
+          Out.push_back(Feature);
+      }
+    }
+
     // Given an index into the 'featuresStrs' sequence, compute a unique
     // ID to be used with function name mangling for the associated variant.
     // This mapping is necessary due to a requirement that the mangling ID
diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index d5312bb171af6c..b136f8ffd659a7 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -477,18 +477,23 @@ ParsedTargetAttr RISCVTargetInfo::parseTargetAttr(StringRef Features) const {
 }
 
 unsigned RISCVTargetInfo::getFMVPriority(ArrayRef<StringRef> Features) const {
-  SmallVector<StringRef, 8> Attrs;
-  Features[0].split(Attrs, ';');
-  // Default Priority is zero.
-  unsigned Priority = 0;
-  for (auto Attr : Attrs) {
-    if (Attr.consume_front("priority=")) {
-      unsigned Result;
-      if (!Attr.getAsInteger(0, Result))
-        Priority = Result;
-    }
+  // Priority is explicitly specified on RISC-V unlike on other targets, where
+  // it is derived by all the features of a specific version. Therefore if a
+  // feature contains the priority string, then return it immediately.
+  for (StringRef Feature : Features) {
+    auto [LHS, RHS] = Feature.rsplit(';');
+    if (LHS.consume_front("priority="))
+      Feature = LHS;
+    else if (RHS.consume_front("priority="))
+      Feature = RHS;
+    else
+      continue;
+    unsigned Priority;
+    if (!Feature.getAsInteger(0, Priority))
+      return Priority;
   }
-  return Priority;
+  // Default Priority is zero.
+  return 0;
 }
 
 TargetInfo::CallingConvCheckResult
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 3311887a4251ba..1b6635c370faa3 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2824,23 +2824,17 @@ void CodeGenFunction::EmitKCFIOperandBundle(
     Bundles.emplace_back("kcfi", CGM.CreateKCFITypeId(FP->desugar()));
 }
 
-llvm::Value *CodeGenFunction::FormAArch64ResolverCondition(
-    const MultiVersionResolverOption &RO) {
-  llvm::SmallVector<StringRef, 8> CondFeatures;
-  for (const StringRef &Feature : RO.Conditions.Features)
-    CondFeatures.push_back(Feature);
-  if (!CondFeatures.empty()) {
-    return EmitAArch64CpuSupports(CondFeatures);
-  }
-  return nullptr;
+llvm::Value *
+CodeGenFunction::FormAArch64ResolverCondition(const FMVResolverOption &RO) {
+  return RO.Features.empty() ? nullptr : EmitAArch64CpuSupports(RO.Features);
 }
 
-llvm::Value *CodeGenFunction::FormX86ResolverCondition(
-    const MultiVersionResolverOption &RO) {
+llvm::Value *
+CodeGenFunction::FormX86ResolverCondition(const FMVResolverOption &RO) {
   llvm::Value *Condition = nullptr;
 
-  if (!RO.Conditions.Architecture.empty()) {
-    StringRef Arch = RO.Conditions.Architecture;
+  if (RO.Architecture) {
+    StringRef Arch = *RO.Architecture;
     // If arch= specifies an x86-64 micro-architecture level, test the feature
     // with __builtin_cpu_supports, otherwise use __builtin_cpu_is.
     if (Arch.starts_with("x86-64"))
@@ -2849,8 +2843,8 @@ llvm::Value *CodeGenFunction::FormX86ResolverCondition(
       Condition = EmitX86CpuIs(Arch);
   }
 
-  if (!RO.Conditions.Features.empty()) {
-    llvm::Value *FeatureCond = EmitX86CpuSupports(RO.Conditions.Features);
+  if (!RO.Features.empty()) {
+    llvm::Value *FeatureCond = EmitX86CpuSupports(RO.Features);
     Condition =
         Condition ? Builder.CreateAnd(Condition, FeatureCond) : FeatureCond;
   }
@@ -2880,7 +2874,7 @@ static void CreateMultiVersionResolverReturn(CodeGenModule &CGM,
 }
 
 void CodeGenFunction::EmitMultiVersionResolver(
-    llvm::Function *Resolver, ArrayRef<MultiVersionResolverOption> Options) {
+    llvm::Function *Resolver, ArrayRef<FMVResolverOption> Options) {
 
   llvm::Triple::ArchType ArchType =
       getContext().getTargetInfo().getTriple().getArch();
@@ -2904,7 +2898,7 @@ void CodeGenFunction::EmitMultiVersionResolver(
 }
 
 void CodeGenFunction::EmitRISCVMultiVersionResolver(
-    llvm::Function *Resolver, ArrayRef<MultiVersionResolverOption> Options) {
+    llvm::Function *Resolver, ArrayRef<FMVResolverOption> Options) {
 
   if (getContext().getTargetInfo().getTriple().getOS() !=
       llvm::Triple::OSType::Linux) {
@@ -2923,7 +2917,7 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
   // Check the each candidate function.
   for (unsigned Index = 0; Index < Options.size(); Index++) {
 
-    if (Options[Index].Conditions.Features[0].starts_with("default")) {
+    if (Options[Index].Features.empty()) {
       HasDefault = true;
       DefaultIndex = Index;
       continue;
@@ -2931,15 +2925,6 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
 
     Builder.SetInsertPoint(CurBlock);
 
-    std::vector<std::string> TargetAttrFeats =
-        getContext()
-            .getTargetInfo()
-            .parseTargetAttr(Options[Index].Conditions.Features[0])
-            .Features;
-
-    if (TargetAttrFeats.empty())
-      continue;
-
     // FeaturesCondition: The bitmask of the required extension has been
     // enabled by the runtime object.
     // (__riscv_feature_bits.features[i] & REQUIRED_BITMASK) ==
@@ -2963,12 +2948,19 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
     // address when using different versions.
     llvm::SmallVector<StringRef, 8> CurrTargetAttrFeats;
 
-    for (auto &Feat : TargetAttrFeats) {
-      StringRef CurrFeat = Feat;
-      if (CurrFeat.starts_with('+'))
-        CurrTargetAttrFeats.push_back(CurrFeat.substr(1));
+    for (StringRef Feat : Options[Index].Features) {
+      auto [LHS, RHS] = Feat.rsplit(';');
+      Feat = LHS.starts_with("priority=") ? RHS : LHS;
+      if (Feat.starts_with("arch="))
+        Feat = Feat.drop_front(sizeof("arch=") - 1);
+      if (Feat.starts_with('+'))
+        Feat = Feat.drop_front(1);
+      CurrTargetAttrFeats.push_back(Feat);
     }
 
+    if (CurrTargetAttrFeats.empty())
+      continue;
+
     Builder.SetInsertPoint(CurBlock);
     llvm::Value *FeatsCondition = EmitRISCVCpuSupports(CurrTargetAttrFeats);
 
@@ -3002,17 +2994,16 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
 }
 
 void CodeGenFunction::EmitAArch64MultiVersionResolver(
-    llvm::Function *Resolver, ArrayRef<MultiVersionResolverOption> Options) {
+    llvm::Function *Resolver, ArrayRef<FMVResolverOption> Options) {
   assert(!Options.empty() && "No multiversion resolver options found");
-  assert(Options.back().Conditions.Features.size() == 0 &&
-         "Default case must be last");
+  assert(Options.back().Features.size() == 0 && "Default case must be last");
   bool SupportsIFunc = getContext().getTargetInfo().supportsIFunc();
   assert(SupportsIFunc &&
          "Multiversion resolver requires target IFUNC support");
   bool AArch64CpuInitialized = false;
   llvm::BasicBlock *CurBlock = createBasicBlock("resolver_entry", Resolver);
 
-  for (const MultiVersionResolverOption &RO : Options) {
+  for (const FMVResolverOption &RO : Options) {
     Builder.SetInsertPoint(CurBlock);
     llvm::Value *Condition = FormAArch64ResolverCondition(RO);
 
@@ -3048,7 +3039,7 @@ void CodeGenFunction::EmitAArch64MultiVersionResolver(
 }
 
 void CodeGenFunction::EmitX86MultiVersionResolver(
-    llvm::Function *Resolver, ArrayRef<MultiVersionResolverOption> Options) {
+    llvm::Function *Resolver, ArrayRef<FMVResolverOption> Options) {
 
   bool SupportsIFunc = getContext().getTargetInfo().supportsIFunc();
 
@@ -3057,7 +3048,7 @@ void CodeGenFunction::EmitX86MultiVersionResolver(
   Builder.SetInsertPoint(CurBlock);
   EmitX86CpuInit();
 
-  for (const MultiVersionResolverOption &RO : Options) {
+  for (const FMVResolverOption &RO : Options) {
     Builder.SetInsertPoint(CurBlock);
     llvm::Value *Condition = FormX86ResolverCondition(RO);
 
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index fcc1013d7361ec..5bfbc278c20f72 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -5332,35 +5332,27 @@ class CodeGenFunction : public CodeGenTypeCache {
 
   void EmitSanitizerStatReport(llvm::SanitizerStatKind SSK);
 
-  struct MultiVersionResolverOption {
+  struct FMVResolverOption {
     llvm::Function *Function;
-    struct Conds {
-      StringRef Architecture;
-      llvm::SmallVector<StringRef, 8> Features;
+    llvm::SmallVector<StringRef, 8> Features;
+    std::optional<StringRef> Architecture;
 
-      Conds(StringRef Arch, ArrayRef<StringRef> Feats)
-          : Architecture(Arch), Features(Feats) {}
-    } Conditions;
-
-    MultiVersionResolverOption(llvm::Function *F, StringRef Arch,
-                               ArrayRef<StringRef> Feats)
-        : Function(F), Conditions(Arch, Feats) {}
+    FMVResolverOption(llvm::Function *F, ArrayRef<StringRef> Feats,
+                      std::optional<StringRef> Arch = std::nullopt)
+        : Function(F), Features(Feats), Architecture(Arch) {}
   };
 
   // Emits the body of a multiversion function's resolver. Assumes that the
   // options are already sorted in the proper order, with the 'default' option
   // last (if it exists).
   void EmitMultiVersionResolver(llvm::Function *Resolver,
-                                ArrayRef<MultiVersionResolverOption> Options);
-  void
-  EmitX86MultiVersionResolver(llvm::Function *Resolver,
-                              ArrayRef<MultiVersionResolverOption> Options);
-  void
-  EmitAArch64MultiVersionResolver(llvm::Function *Resolver,
-                                  ArrayRef<MultiVersionResolverOption> Options);
-  void
-  EmitRISCVMultiVersionResolver(llvm::Function *Resolver,
-                                ArrayRef<MultiVersionResolverOption> Options);
+                                ArrayRef<FMVResolverOption> Options);
+  void EmitX86MultiVersionResolver(llvm::Function *Resolver,
+                                   ArrayRef<FMVResolverOption> Options);
+  void EmitAArch64MultiVersionResolver(llvm::Function *Resolver,
+                                       ArrayRef<FMVResolverOption> Options);
+  void EmitRISCVMultiVersionResolver(llvm::Function *Resolver,
+                                     ArrayRef<FMVResolverOption> Options);
 
 private:
   QualType getVarArgType(const Expr *Arg);
@@ -5379,10 +5371,9 @@ class CodeGenFunction : public CodeGenTypeCache {
   llvm::Value *EmitX86CpuSupports(ArrayRef<StringRef> FeatureStrs);
   llvm::Value *EmitX86CpuSupports(std::array<uint32_t, 4> FeatureMask);
   llvm::Value *EmitX86CpuInit();
-  llvm::Value *FormX86ResolverCondition(const MultiVersionResolverOption &RO);
+  llvm::Value *FormX86ResolverCondition(const FMVResolverOption &RO);
   llvm::Value *EmitAArch64CpuInit();
-  llvm::Value *
-  FormAArch64ResolverCondition(const MultiVersionResolverOption &RO);
+  llvm::Value *FormAArch64ResolverCondition(const FMVResolverOption &RO);
   llvm::Value *EmitAArch64CpuSupports(const CallExpr *E);
   llvm::Value *EmitAArch64CpuSupports(ArrayRef<StringRef> FeatureStrs);
 };
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 171ce4f47b2867..0fc4aa8fd4781c 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4211,11 +4211,11 @@ void CodeGenModule::EmitGlobalDefinition(GlobalDecl GD, llvm::GlobalValue *GV) {
 static void ReplaceUsesOfNonProtoTypeWithRealFunction(llvm::GlobalValue *Old,
                                                       llvm::Function *NewFn);
 
-static unsigned
-getFMVPriority(const TargetInfo &TI,
-               const CodeGenFunction::MultiVersionResolverOption &RO) {
-  llvm::SmallVector<StringRef, 8> Features{RO.Conditions.Features};
-  Features.push_back(RO.Conditions.Architecture);
+static unsigned getFMVPriority(const TargetInfo &TI,
+                               const CodeGenFunction::FMVResolverOption &RO) {
+  llvm::SmallVector<StringRef, 8> Features{RO.Features};
+  if (RO.Architecture)
+    Features.push_back(*RO.Architecture);
   return TI.getFMVPriority(Features);
 }
 
@@ -4262,7 +4262,7 @@ void CodeGenModule::emitMultiVersionFunctions() {
     // target_version("default")) or target_clones() is present and defined
     // in this TU. For other architectures it is always emitted.
     bool ShouldEmitResolver = !getTarget().getTriple().isAArch64();
-    SmallVector<CodeGenFunction::MultiVersionResolverOption, 10> Options;
+    SmallVector<CodeGenFunction::FMVResolverOption, 10> Options;
 
     getContext().forEachMultiversionedFunctionVersion(
         FD, [&](const FunctionDecl *CurFD) {
@@ -4272,18 +4272,14 @@ void CodeGenModule::emitMultiVersionFunctions() {
           if (const auto *TA = CurFD->getAttr<TargetAttr>()) {
             TA->getAddedFeatures(Feats);
             llvm::Function *Func = createFunction(CurFD);
-            Options.emplace_back(Func, TA->getArchitecture(), Feats);
+            Options.emplace_back(Func, Feats, TA->getArchitecture());
           } else if (const auto *TVA = CurFD->getAttr<TargetVersionAttr>()) {
             if (TVA->isDefaultVersion() && IsDefined)
               ShouldEmitResolver = true;
             llvm::Function *Func = createFunction(CurFD);
-            if (getTarget().getTriple().isRISCV()) {
-              Feats.push_back(TVA->getName());
-            } else {
-              assert(getTarget().getTriple().isAArch64());
-              TVA->getFeatures(Feats);
-            }
-            Options.emplace_back(Func, /*Architecture*/ "", Feats);
+            char Delim = getTarget().getTriple().isAArch64() ? '+' : ',';
+            TVA->getFeatures(Feats, Delim);
+            Options.emplace_back(Func, Feats);
           } else if (const auto *TC = CurFD->getAttr<TargetClonesAttr>()) {
             if (IsDefined)
               ShouldEmitResolver = true;
@@ -4292,21 +4288,15 @@ void CodeGenModule::emitMultiVersionFunctions() {
                 continue;
 
               llvm::Function *Func = createFunction(CurFD, I);
-              StringRef Architecture;
               Feats.clear();
-              if (getTarget().getTriple().isAArch64())
-                TC->getFeatures(Feats, I);
-              else if (getTarget().getTriple().isRISCV()) {
-                StringRef Version = TC->getFeatureStr(I);
-                Feats.push_back(Version);
+              if (getTarget().getTriple().isX86()) {
+                TC->getAddedFeatures(Feats, I);
+                Options.emplace_back(Func, Feats, TC->getArchitecture(I));
               } else {
-                StringRef Version = TC->getFeatureStr(I);
-                if (Version.starts_with("arch="))
-                  Architecture = Version.drop_front(sizeof("arch=") - 1);
-                else if (Version != "default")
-                  Feats.push_back(Version);
+                char Delim = getTarget().getTriple().isAArch64() ? '+' : ',';
+                TC->getFeatures(Feats, I, Delim);
+                Options.emplace_back(Func, Feats);
               }
-              Options.emplace_back(Func, Architecture, Feats);
             }
           } else
             llvm_unreachable("unexpected MultiVersionKind");
@@ -4345,8 +4335,8 @@ void CodeGenModule::emitMultiVersionFunctions() {
 
     const TargetInfo &TI = getTarget();
     llvm::stable_sort(
-        Options, [&TI](const CodeGenFunction::MultiVersionResolverOption &LHS,
-                       const CodeGenFunction::MultiVersionResolverOption &RHS) {
+        Options, [&TI](const CodeGenFunction::FMVResolverOption &LHS,
+                       const CodeGenFunction::FMVResolverOption &RHS) {
           return getFMVPriority(TI, LHS) > getFMVPriority(TI, RHS);
         });
     CodeGenFunction CGF(*this);
@@ -4406,7 +4396,7 @@ void CodeGenModule::emitCPUDispatchDefinition(GlobalDecl GD) {
     ResolverFunc->setComdat(
         getModule().getOrInsertComdat(ResolverFunc->getName()));
 
-  SmallVector<CodeGenFunction::MultiVersionResolverOption, 10> Options;
+  SmallVector<CodeGenFunction::FMVResolverOption, 10> Options;
   const TargetInfo &Target = getTarget();
   unsigned Index = 0;
   for (const IdentifierInfo *II : DD->cpus()) {
@@ -4440,25 +4430,23 @@ void CodeGenModule::emitCPUDispatchDefinition(GlobalDecl GD) {
     llvm::erase_if(Features, [&Target](StringRef Feat) {
       return !Target.validateCpuSupports(Feat);
     });
-    Options.emplace_back(cast<llvm::Function>(Func), StringRef{}, Features);
+    Options.emplace_back(cast<llvm::Function>(Func), Features);
     ++Index;
   }
 
-  llvm::stable_sort(
-      Options, [](const CodeGenFunction::MultiVersionResolverOption &LHS,
-                  const CodeGenFunction::MultiVersionResolverOption &RHS) {
-        return llvm::X86::getCpuSupportsMask(LHS.Conditions.Features) >
-               llvm::X86::getCpuSupportsMask(RHS.Conditions.Features);
-      });
+  llvm::stable_sort(Options, [](const CodeGenFunction::FMVResolverOption &LHS,
+                                const CodeGenFunction::FMVResolverOption &RHS) {
+    return llvm::X86::getCpuSupportsMask(LHS.Features) >
+           llvm::X86::getCpuSupportsMask(RHS.Features);
+  });
 
   // If the list contains multiple 'default' versions, such as when it contains
   // 'pentium' and 'generic', don't emit the call to the generic one (since we
   // always run on at least a 'pentium'). We do this by deleting the 'least
   // advanced' (read, lowest mangling letter).
-  while (Options.size() > 1 &&
-         llvm::all_of(llvm::X86::getCpuSupportsMask(
-                          (Options.end() - 2)->Conditions.Features),
-                      [](auto X) { return X == 0; })) {
+  while (Options.size() > 1 && llvm::all_of(llvm::X86::getCpuSupportsMask(
+                                                (Options.end() - 2)->Features),
+                                            [](auto X) { return X == 0; })) {
     StringRef LHSName = (Options.end() - 2)->Function->getName();
     StringRef RHSName = (Options.end() - 1)->Function->getName();
     if (LHSName.compare(RHSName) < 0)

>From b093fb82b67d596f025c2acc37983db5ef1f95de Mon Sep 17 00:00:00 2001
From: Alexandros Lamprineas <alexandros.lamprineas at arm.com>
Date: Tue, 26 Nov 2024 15:07:36 +0000
Subject: [PATCH 5/6] Changes from last revision:

* Do not split by ',' the target_clones attribute on x86.
* Rename x86 specific parsing methods.
---
 clang/include/clang/Basic/Attr.td   | 46 ++++++++++-------------------
 clang/lib/CodeGen/CodeGenModule.cpp |  9 +++---
 2 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 4dccce7cd9e0f7..9dcbf23fea9400 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3260,10 +3260,10 @@ def Target : InheritableAttr {
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [TargetDocs];
   let AdditionalMembers = [{
-    std::optional<StringRef> getArchitecture() const {
+    std::optional<StringRef> getX86Architecture() const {
       StringRef Features = getFeaturesStr();
       SmallVector<StringRef, 4> AttrFeatures;
-      Features.split(AttrFeatures, ",");
+      Features.split(AttrFeatures, ',');
       for (StringRef Feature : AttrFeatures) {
         Feature = Feature.trim();
         if (Feature.starts_with("arch="))
@@ -3274,16 +3274,14 @@ def Target : InheritableAttr {
 
     // Gets the list of features as simple string-refs with no +/- or 'no-'.
     // Only adds the items to 'Out' that are additions.
-    void getAddedFeatures(llvm::SmallVectorImpl<StringRef> &Out) const {
+    void getX86AddedFeatures(llvm::SmallVectorImpl<StringRef> &Out) const {
+      if (isDefaultVersion())
+        return;
       StringRef Features = getFeaturesStr();
-      if (Features == "default") return;
-
-      SmallVector<StringRef, 1> AttrFeatures;
-      Features.split(AttrFeatures, ",");
-
+      SmallVector<StringRef, 4> AttrFeatures;
+      Features.split(AttrFeatures, ',');
       for (auto &Feature : AttrFeatures) {
         Feature = Feature.trim();
-
         if (!Feature.starts_with("no-") && !Feature.starts_with("arch=") &&
             !Feature.starts_with("fpmath=") && !Feature.starts_with("tune="))
           Out.push_back(Feature);
@@ -3346,32 +3344,20 @@ def TargetClones : InheritableAttr {
       }
     }
 
-    // x86 only.
-    std::optional<StringRef> getArchitecture(unsigned Index) const {
-      StringRef Features = getFeatureStr(Index);
-      SmallVector<StringRef, 4> AttrFeatures;
-      Features.split(AttrFeatures, ',');
-      for (StringRef Feature : AttrFeatures) {
-        Feature = Feature.trim();
-        if (Feature.starts_with("arch="))
-          return Feature.drop_front(sizeof("arch=") - 1);
-      }
+    std::optional<StringRef> getX86Architecture(unsigned Index) const {
+      StringRef Feature = getFeatureStr(Index);
+      if (Feature.starts_with("arch="))
+        return Feature.drop_front(sizeof("arch=") - 1);
       return std::nullopt;
     }
 
-    // x86 only.
-    void getAddedFeatures(llvm::SmallVectorImpl<StringRef> &Out,
-                          unsigned Index) const {
+    void getX86Feature(llvm::SmallVectorImpl<StringRef> &Out,
+                       unsigned Index) const {
       if (isDefaultVersion(Index))
         return;
-      StringRef Features = getFeatureStr(Index);
-      SmallVector<StringRef, 4> AttrFeatures;
-      Features.split(AttrFeatures, ",");
-      for (StringRef Feature : AttrFeatures) {
-        Feature = Feature.trim();
-        if (!Feature.starts_with("arch="))
-          Out.push_back(Feature);
-      }
+      if (getX86Architecture(Index))
+        return;
+      Out.push_back(getFeatureStr(Index));
     }
 
     // Given an index into the 'featuresStrs' sequence, compute a unique
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 0fc4aa8fd4781c..a11b9b740a9f59 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4270,9 +4270,10 @@ void CodeGenModule::emitMultiVersionFunctions() {
           bool IsDefined = CurFD->doesThisDeclarationHaveABody();
 
           if (const auto *TA = CurFD->getAttr<TargetAttr>()) {
-            TA->getAddedFeatures(Feats);
+            assert(getTarget().getTriple().isX86() && "Unsupported target");
+            TA->getX86AddedFeatures(Feats);
             llvm::Function *Func = createFunction(CurFD);
-            Options.emplace_back(Func, Feats, TA->getArchitecture());
+            Options.emplace_back(Func, Feats, TA->getX86Architecture());
           } else if (const auto *TVA = CurFD->getAttr<TargetVersionAttr>()) {
             if (TVA->isDefaultVersion() && IsDefined)
               ShouldEmitResolver = true;
@@ -4290,8 +4291,8 @@ void CodeGenModule::emitMultiVersionFunctions() {
               llvm::Function *Func = createFunction(CurFD, I);
               Feats.clear();
               if (getTarget().getTriple().isX86()) {
-                TC->getAddedFeatures(Feats, I);
-                Options.emplace_back(Func, Feats, TC->getArchitecture(I));
+                TC->getX86Feature(Feats, I);
+                Options.emplace_back(Func, Feats, TC->getX86Architecture(I));
               } else {
                 char Delim = getTarget().getTriple().isAArch64() ? '+' : ',';
                 TC->getFeatures(Feats, I, Delim);

>From 9386ac6aad19bf0f709f5db2bf16328aa9e80bf7 Mon Sep 17 00:00:00 2001
From: Alexandros Lamprineas <alexandros.lamprineas at arm.com>
Date: Wed, 27 Nov 2024 15:05:09 +0000
Subject: [PATCH 6/6] Changes from last revision:

Use parseTargetAttr for already delimited attribute strings.
---
 clang/lib/Basic/Targets/RISCV.cpp     | 37 +++++++++++++++++----------
 clang/lib/CodeGen/CodeGenFunction.cpp | 24 +++++++++++------
 2 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index b136f8ffd659a7..d67d5d56e3fdf1 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -419,6 +419,24 @@ ParsedTargetAttr RISCVTargetInfo::parseTargetAttr(StringRef Features) const {
   Features.split(AttrFeatures, ";");
   bool FoundArch = false;
 
+  auto handleArchExtension = [](StringRef AttrString,
+                                std::vector<std::string> &Features) {
+    SmallVector<StringRef, 1> Exts;
+    AttrString.split(Exts, ",");
+    for (auto Ext : Exts) {
+      if (Ext.empty())
+        continue;
+
+      StringRef ExtName = Ext.substr(1);
+      std::string TargetFeature =
+          llvm::RISCVISAInfo::getTargetFeatureForExtension(ExtName);
+      if (!TargetFeature.empty())
+        Features.push_back(Ext.front() + TargetFeature);
+      else
+        Features.push_back(Ext.str());
+    }
+  };
+
   for (auto &Feature : AttrFeatures) {
     Feature = Feature.trim();
     StringRef AttrString = Feature.split("=").second.trim();
@@ -432,20 +450,7 @@ ParsedTargetAttr RISCVTargetInfo::parseTargetAttr(StringRef Features) const {
 
       if (AttrString.starts_with("+")) {
         // EXTENSION like arch=+v,+zbb
-        SmallVector<StringRef, 1> Exts;
-        AttrString.split(Exts, ",");
-        for (auto Ext : Exts) {
-          if (Ext.empty())
-            continue;
-
-          StringRef ExtName = Ext.substr(1);
-          std::string TargetFeature =
-              llvm::RISCVISAInfo::getTargetFeatureForExtension(ExtName);
-          if (!TargetFeature.empty())
-            Ret.Features.push_back(Ext.front() + TargetFeature);
-          else
-            Ret.Features.push_back(Ext.str());
-        }
+        handleArchExtension(AttrString, Ret.Features);
       } else {
         // full-arch-string like arch=rv64gcv
         handleFullArchString(AttrString, Ret.Features);
@@ -471,6 +476,10 @@ ParsedTargetAttr RISCVTargetInfo::parseTargetAttr(StringRef Features) const {
       Ret.Tune = AttrString;
     } else if (Feature.starts_with("priority")) {
       // Skip because it only use for FMV.
+    } else if (Feature.starts_with("+")) {
+      // Handle target_version/target_clones attribute strings
+      // that are already delimited by ','
+      handleArchExtension(Feature, Ret.Features);
     }
   }
   return Ret;
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 1b6635c370faa3..6b04e3522bf8b3 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2947,20 +2947,28 @@ void CodeGenFunction::EmitRISCVMultiVersionResolver(
     // Without checking the length first, we may access an incorrect memory
     // address when using different versions.
     llvm::SmallVector<StringRef, 8> CurrTargetAttrFeats;
+    llvm::SmallVector<std::string, 8> TargetAttrFeats;
 
     for (StringRef Feat : Options[Index].Features) {
-      auto [LHS, RHS] = Feat.rsplit(';');
-      Feat = LHS.starts_with("priority=") ? RHS : LHS;
-      if (Feat.starts_with("arch="))
-        Feat = Feat.drop_front(sizeof("arch=") - 1);
-      if (Feat.starts_with('+'))
-        Feat = Feat.drop_front(1);
-      CurrTargetAttrFeats.push_back(Feat);
+      std::vector<std::string> FeatStr =
+          getContext()
+              .getTargetInfo()
+              .parseTargetAttr(Feat)
+              .Features;
+
+      assert(FeatStr.size() == 1 && "Feature string not delimited");
+
+      std::string &CurrFeat = FeatStr.front();
+      if (CurrFeat[0] == '+')
+        TargetAttrFeats.push_back(CurrFeat.substr(1));
     }
 
-    if (CurrTargetAttrFeats.empty())
+    if (TargetAttrFeats.empty())
       continue;
 
+    for (std::string &Feat : TargetAttrFeats)
+      CurrTargetAttrFeats.push_back(Feat);
+
     Builder.SetInsertPoint(CurBlock);
     llvm::Value *FeatsCondition = EmitRISCVCpuSupports(CurrTargetAttrFeats);
 



More information about the cfe-commits mailing list