[clang] [RISCV] Overwrite cpu target features for full arch string in target attribute (PR #77426)

Luke Lau via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 16 20:50:01 PST 2024


https://github.com/lukel97 updated https://github.com/llvm/llvm-project/pull/77426

>From 0fadce20076015fbb28d449a2b3086f2e4261604 Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Tue, 9 Jan 2024 15:32:15 +0700
Subject: [PATCH 1/4] [RISCV] Overwrite cpu target features for full arch
 string in target attribute

This patch reworks RISCVTargetInfo::initFeatureMap to fix the issue described
in https://github.com/llvm/llvm-project/pull/74889#pullrequestreview-1773445559
(and is an alternative to #75804)

When a full arch string is specified, a "full" list of extensions is now passed
after the __RISCV_TargetAttrNeedOverride marker feature, which includes any
negative features that disable ISA extensions.

In initFeatureMap, there are now two code paths:

1. If the arch string was overriden, use the "full" list of override features,
only adding back any non-isa features that were specified.

Using the full list of positive and negative features will mean that the
target-cpu will have no effect on the final arch, e.g.
__attribute__((target("arch=rv64i"))) with -mcpu=sifive-x280 will have the
features for rv64i, not a mix of both.

2. Otherwise, parse and *append* the list of implied features. By appending, we
turn back on any features that might have been disabled by a negative
extension, i.e. this handles the case fixed in #74889.
---
 clang/lib/Basic/Targets/RISCV.cpp             | 78 +++++++------------
 .../CodeGen/RISCV/riscv-func-attr-target.c    |  8 +-
 2 files changed, 30 insertions(+), 56 deletions(-)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index daaa8639ae8358..b56c1d465ad77a 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -235,39 +235,6 @@ ArrayRef<Builtin::Info> RISCVTargetInfo::getTargetBuiltins() const {
                         clang::RISCV::LastTSBuiltin - Builtin::FirstTSBuiltin);
 }
 
-static std::vector<std::string>
-collectNonISAExtFeature(ArrayRef<std::string> FeaturesNeedOverride, int XLen) {
-  std::vector<std::string> NonISAExtFeatureVec;
-
-  auto IsNonISAExtFeature = [](const std::string &Feature) {
-    assert(Feature.size() > 1 && (Feature[0] == '+' || Feature[0] == '-'));
-    StringRef Ext = StringRef(Feature).drop_front(); // drop the +/-
-    return !llvm::RISCVISAInfo::isSupportedExtensionFeature(Ext);
-  };
-  llvm::copy_if(FeaturesNeedOverride, std::back_inserter(NonISAExtFeatureVec),
-                IsNonISAExtFeature);
-
-  return NonISAExtFeatureVec;
-}
-
-static std::vector<std::string>
-resolveTargetAttrOverride(const std::vector<std::string> &FeaturesVec,
-                          int XLen) {
-  auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
-  if (I == FeaturesVec.end())
-    return FeaturesVec;
-
-  ArrayRef<std::string> FeaturesNeedOverride(&*FeaturesVec.begin(), &*I);
-  std::vector<std::string> NonISAExtFeature =
-      collectNonISAExtFeature(FeaturesNeedOverride, XLen);
-
-  std::vector<std::string> ResolvedFeature(++I, FeaturesVec.end());
-  ResolvedFeature.insert(ResolvedFeature.end(), NonISAExtFeature.begin(),
-                         NonISAExtFeature.end());
-
-  return ResolvedFeature;
-}
-
 bool RISCVTargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeaturesVec) const {
@@ -281,10 +248,27 @@ bool RISCVTargetInfo::initFeatureMap(
     Features["32bit"] = true;
   }
 
-  std::vector<std::string> NewFeaturesVec =
-      resolveTargetAttrOverride(FeaturesVec, XLen);
+  // If a target attribute specified a full arch string, override all the ISA
+  // extension target features.
+  const auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
+  if (I != FeaturesVec.end()) {
+    std::vector OverrideFeatures = std::vector(std::next(I), FeaturesVec.end());
+
+    // Add back any non ISA extension features, e.g. +relax.
+    auto IsNonISAExtFeature = [](const std::string &Feature) {
+      assert(Feature.size() > 1 && (Feature[0] == '+' || Feature[0] == '-'));
+      std::string Ext = Feature.substr(1); // drop the +/-
+      return !llvm::RISCVISAInfo::isSupportedExtensionFeature(Ext);
+    };
+    llvm::copy_if(llvm::make_range(FeaturesVec.begin(), I),
+                  std::back_inserter(OverrideFeatures), IsNonISAExtFeature);
 
-  auto ParseResult = llvm::RISCVISAInfo::parseFeatures(XLen, NewFeaturesVec);
+    return TargetInfo::initFeatureMap(Features, Diags, CPU, OverrideFeatures);
+  }
+
+  // Otherwise, parse the features and add any implied extensions.
+  std::vector AllFeatures = FeaturesVec;
+  auto ParseResult = llvm::RISCVISAInfo::parseFeatures(XLen, FeaturesVec);
   if (!ParseResult) {
     std::string Buffer;
     llvm::raw_string_ostream OutputErrMsg(Buffer);
@@ -295,21 +279,9 @@ bool RISCVTargetInfo::initFeatureMap(
     return false;
   }
 
-  // RISCVISAInfo makes implications for ISA features
-  std::vector<std::string> ImpliedFeatures = (*ParseResult)->toFeatures();
-
-  // parseFeatures normalizes the feature set by dropping any explicit
-  // negatives, and non-extension features.  We need to preserve the later
-  // for correctness and want to preserve the former for consistency.
-  for (auto &Feature : NewFeaturesVec) {
-     StringRef ExtName = Feature;
-     assert(ExtName.size() > 1 && (ExtName[0] == '+' || ExtName[0] == '-'));
-     ExtName = ExtName.drop_front(1); // Drop '+' or '-'
-     if (!llvm::is_contained(ImpliedFeatures, ("+" + ExtName).str()) &&
-         !llvm::is_contained(ImpliedFeatures, ("-" + ExtName).str()))
-       ImpliedFeatures.push_back(Feature);
-  }
-  return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
+  // Append all features, not just new ones, so we override any negatives.
+  llvm::append_range(AllFeatures, (*ParseResult)->toFeatures());
+  return TargetInfo::initFeatureMap(Features, Diags, CPU, AllFeatures);
 }
 
 std::optional<std::pair<unsigned, unsigned>>
@@ -413,7 +385,9 @@ static void handleFullArchString(StringRef FullArchStr,
     // Forward the invalid FullArchStr.
     Features.push_back("+" + FullArchStr.str());
   } else {
-    std::vector<std::string> FeatStrings = (*RII)->toFeatures();
+    // Append a full list of features, including any negative extensions so that
+    // we override the CPU's features.
+    std::vector<std::string> FeatStrings = (*RII)->toFeatures(true);
     Features.insert(Features.end(), FeatStrings.begin(), FeatStrings.end());
   }
 }
diff --git a/clang/test/CodeGen/RISCV/riscv-func-attr-target.c b/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
index 759c33a2250600..54a0aad8a18b43 100644
--- a/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
+++ b/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
@@ -40,8 +40,8 @@ __attribute__((target("cpu=sifive-u54"))) void testAttrCpuOnly() {}
 // CHECK: attributes #1 = { {{.*}}"target-cpu"="rocket-rv64" "target-features"="+64bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="generic-rv64" }
 // CHECK: attributes #2 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei,-relax,-zfa" }
 // CHECK: attributes #3 = { {{.*}}"target-features"="+64bit,+a,+d,+experimental-zicond,+f,+m,+save-restore,+v,+zbb,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zfa" }
-// CHECK: attributes #4 = { {{.*}}"target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zbb,+zicsr,+zifencei,-relax" }
-// CHECK: attributes #5 = { {{.*}}"target-features"="+64bit,+m,+save-restore,-relax" }
+// CHECK: attributes #4 = { {{.*}}"target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zbb,+zicsr,+zifencei,-e,-experimental-zacas,-experimental-zcmop,-experimental-zfbfmin,-experimental-zicfilp,-experimental-zicfiss,-experimental-zicond,-experimental-zimop,-experimental-ztso,-experimental-zvfbfmin,-experimental-zvfbfwma,-h,-relax,-smaia,-ssaia,-svinval,-svnapot,-svpbmt,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-zawrs,-zba,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmp,-zcmt,-zdinx,-zfa,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zicbom,-zicbop,-zicboz,-zicntr,-zihintntl,-zihintpause,-zihpm,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-zmmul,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b" }
+// CHECK: attributes #5 = { {{.*}}"target-features"="+64bit,+m,+save-restore,-a,-c,-d,-e,-experimental-zacas,-experimental-zcmop,-experimental-zfbfmin,-experimental-zicfilp,-experimental-zicfiss,-experimental-zicond,-experimental-zimop,-experimental-ztso,-experimental-zvfbfmin,-experimental-zvfbfwma,-f,-h,-relax,-smaia,-ssaia,-svinval,-svnapot,-svpbmt,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-zawrs,-zba,-zbb,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmp,-zcmt,-zdinx,-zfa,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zicbom,-zicbop,-zicboz,-zicntr,-zicsr,-zifencei,-zihintntl,-zihintpause,-zihpm,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-zmmul,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b" }
 // CHECK: attributes #6 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei,-relax,-zfa" }
-// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+m,+save-restore,-relax" }
-// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,-relax" }
+// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+m,+save-restore,-a,-c,-d,-e,-experimental-zacas,-experimental-zcmop,-experimental-zfbfmin,-experimental-zicfilp,-experimental-zicfiss,-experimental-zicond,-experimental-zimop,-experimental-ztso,-experimental-zvfbfmin,-experimental-zvfbfwma,-f,-h,-relax,-smaia,-ssaia,-svinval,-svnapot,-svpbmt,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-zawrs,-zba,-zbb,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmp,-zcmt,-zdinx,-zfa,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zicbom,-zicbop,-zicboz,-zicntr,-zicsr,-zifencei,-zihintntl,-zihintpause,-zihpm,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-zmmul,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b" }
+// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,-e,-experimental-zacas,-experimental-zcmop,-experimental-zfbfmin,-experimental-zicfilp,-experimental-zicfiss,-experimental-zicond,-experimental-zimop,-experimental-ztso,-experimental-zvfbfmin,-experimental-zvfbfwma,-h,-relax,-smaia,-ssaia,-svinval,-svnapot,-svpbmt,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-zawrs,-zba,-zbb,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmp,-zcmt,-zdinx,-zfa,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zicbom,-zicbop,-zicboz,-zicntr,-zihintntl,-zihintpause,-zihpm,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-zmmul,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b" }

>From 82536a4d13e085803da255ebcf654ac8281e5f7e Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Mon, 15 Jan 2024 12:20:25 +0700
Subject: [PATCH 2/4] Use std::vector<std::string>, add comment for
 AddAllExtensions argument

---
 clang/lib/Basic/Targets/RISCV.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index b56c1d465ad77a..c46be2300bcb54 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -252,7 +252,8 @@ bool RISCVTargetInfo::initFeatureMap(
   // extension target features.
   const auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
   if (I != FeaturesVec.end()) {
-    std::vector OverrideFeatures = std::vector(std::next(I), FeaturesVec.end());
+    std::vector<std::string> OverrideFeatures =
+        std::vector(std::next(I), FeaturesVec.end());
 
     // Add back any non ISA extension features, e.g. +relax.
     auto IsNonISAExtFeature = [](const std::string &Feature) {
@@ -387,7 +388,8 @@ static void handleFullArchString(StringRef FullArchStr,
   } else {
     // Append a full list of features, including any negative extensions so that
     // we override the CPU's features.
-    std::vector<std::string> FeatStrings = (*RII)->toFeatures(true);
+    std::vector<std::string> FeatStrings =
+        (*RII)->toFeatures(/* AddAllExtensions */ true);
     Features.insert(Features.end(), FeatStrings.begin(), FeatStrings.end());
   }
 }

>From 33fe75c57cefee260c3a072cd768dcf8250d83f7 Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Wed, 17 Jan 2024 11:01:47 +0700
Subject: [PATCH 3/4] Use StringRef, use direct initialization for std::vector

---
 clang/lib/Basic/Targets/RISCV.cpp | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index c46be2300bcb54..90ffd421c25d17 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -252,13 +252,12 @@ bool RISCVTargetInfo::initFeatureMap(
   // extension target features.
   const auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
   if (I != FeaturesVec.end()) {
-    std::vector<std::string> OverrideFeatures =
-        std::vector(std::next(I), FeaturesVec.end());
+    std::vector<std::string> OverrideFeatures(std::next(I), FeaturesVec.end());
 
     // Add back any non ISA extension features, e.g. +relax.
-    auto IsNonISAExtFeature = [](const std::string &Feature) {
+    auto IsNonISAExtFeature = [](StringRef Feature) {
       assert(Feature.size() > 1 && (Feature[0] == '+' || Feature[0] == '-'));
-      std::string Ext = Feature.substr(1); // drop the +/-
+      StringRef Ext = Feature.substr(1); // drop the +/-
       return !llvm::RISCVISAInfo::isSupportedExtensionFeature(Ext);
     };
     llvm::copy_if(llvm::make_range(FeaturesVec.begin(), I),

>From 9f270a63e591d28a913ed895758b95e1c96c23eb Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Wed, 17 Jan 2024 11:49:10 +0700
Subject: [PATCH 4/4] Add more explicit template arguments to std::vector

---
 clang/lib/Basic/Targets/RISCV.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index 90ffd421c25d17..6bd308728f082e 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -267,7 +267,7 @@ bool RISCVTargetInfo::initFeatureMap(
   }
 
   // Otherwise, parse the features and add any implied extensions.
-  std::vector AllFeatures = FeaturesVec;
+  std::vector<std::string> AllFeatures = FeaturesVec;
   auto ParseResult = llvm::RISCVISAInfo::parseFeatures(XLen, FeaturesVec);
   if (!ParseResult) {
     std::string Buffer;



More information about the cfe-commits mailing list