[Clang][RISCV] Recognize unsupport target feature by supporting isValidFeatureName #106495

BeMg · 2024-08-29T05:38:10Z

This patch makes unsupported target attributes emit a warning and ignore the target attribute during semantic checks. The changes include:

Adding the RISCVTargetInfo::isValidFeatureName function.
Rejecting non-full-arch strings in the handleFullArchString function.
Adding test cases to demonstrate the warning behavior.

llvmbot · 2024-08-29T05:38:39Z

@llvm/pr-subscribers-backend-risc-v

Author: Piyou Chen (BeMg)

Changes

…isValidFeatureName

This patch makes unsupported target attributes emit a warning and ignore the target attribute during semantic checks. The changes include:

Adding the RISCVTargetInfo::isValidFeatureName function.
Adding a '+' sign to __RISCV_TargetAttrNeedOverride because checkTargetAttr will remove the sign.
Rejecting non-full-arch strings in the handleFullArchString function.
Adding test cases to demonstrate the warning behavior.

Full diff: https://github.com/llvm/llvm-project/pull/106495.diff

4 Files Affected:

(modified) clang/lib/Basic/Targets/RISCV.cpp (+16-2)
(modified) clang/lib/Basic/Targets/RISCV.h (+1)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+11-4)
(modified) clang/test/Sema/attr-target-riscv.c (+9)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index 1f8a8cd1462c9d..1d6f49ca232692 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -257,7 +257,7 @@ bool RISCVTargetInfo::initFeatureMap(
 
   // If a target attribute specified a full arch string, override all the ISA
   // extension target features.
-  const auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
+  const auto I = llvm::find(FeaturesVec, "+__RISCV_TargetAttrNeedOverride");
   if (I != FeaturesVec.end()) {
     std::vector<std::string> OverrideFeatures(std::next(I), FeaturesVec.end());
 
@@ -391,7 +391,14 @@ void RISCVTargetInfo::fillValidTuneCPUList(
 
 static void handleFullArchString(StringRef FullArchStr,
                                  std::vector<std::string> &Features) {
-  Features.push_back("__RISCV_TargetAttrNeedOverride");
+
+  // Should be full arch string.
+  if (!FullArchStr.starts_with("rv")) {
+    Features.push_back(FullArchStr.str());
+    return;
+  }
+
+  Features.push_back("+__RISCV_TargetAttrNeedOverride");
   auto RII = llvm::RISCVISAInfo::parseArchString(
       FullArchStr, /* EnableExperimentalExtension */ true);
   if (llvm::errorToBool(RII.takeError())) {
@@ -485,3 +492,10 @@ bool RISCVTargetInfo::validateCpuSupports(StringRef Feature) const {
   // __riscv_feature_bits structure.
   return -1 != llvm::RISCVISAInfo::getRISCVFeaturesBitsInfo(Feature).second;
 }
+
+bool RISCVTargetInfo::isValidFeatureName(StringRef Name) const {
+  if (Name == "__RISCV_TargetAttrNeedOverride")
+    return true;
+
+  return llvm::RISCVISAInfo::isSupportedExtensionFeature(Name);
+}
diff --git a/clang/lib/Basic/Targets/RISCV.h b/clang/lib/Basic/Targets/RISCV.h
index 626274b8fc437c..b808ccc8e9cfe9 100644
--- a/clang/lib/Basic/Targets/RISCV.h
+++ b/clang/lib/Basic/Targets/RISCV.h
@@ -130,6 +130,7 @@ class RISCVTargetInfo : public TargetInfo {
   bool supportsCpuSupports() const override { return getTriple().isOSLinux(); }
   bool supportsCpuInit() const override { return getTriple().isOSLinux(); }
   bool validateCpuSupports(StringRef Feature) const override;
+  bool isValidFeatureName(StringRef Name) const override;
 };
 class LLVM_LIBRARY_VISIBILITY RISCV32TargetInfo : public RISCVTargetInfo {
 public:
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 1e074298ac5289..81cff8d7362ad5 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2993,10 +2993,17 @@ bool Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
            << Unknown << Tune << ParsedAttrs.Tune << Target;
 
-  if (Context.getTargetInfo().getTriple().isRISCV() &&
-      ParsedAttrs.Duplicate != "")
-    return Diag(LiteralLoc, diag::err_duplicate_target_attribute)
-           << Duplicate << None << ParsedAttrs.Duplicate << Target;
+  if (Context.getTargetInfo().getTriple().isRISCV()) {
+    if (ParsedAttrs.Duplicate != "")
+      return Diag(LiteralLoc, diag::err_duplicate_target_attribute)
+             << Duplicate << None << ParsedAttrs.Duplicate << Target;
+    for (const auto &Feature : ParsedAttrs.Features) {
+      auto CurFeature = StringRef(Feature);
+      if (!CurFeature.starts_with("+") && !CurFeature.starts_with("-"))
+        return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+               << Unsupported << None << AttrStr << Target;
+    }
+  }
 
   if (ParsedAttrs.Duplicate != "")
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
diff --git a/clang/test/Sema/attr-target-riscv.c b/clang/test/Sema/attr-target-riscv.c
index ed4e2915d6c6ef..01d928c1d78e48 100644
--- a/clang/test/Sema/attr-target-riscv.c
+++ b/clang/test/Sema/attr-target-riscv.c
@@ -4,3 +4,12 @@
 int __attribute__((target("arch=rv64g"))) foo(void) { return 0; }
 //expected-error@+1 {{redefinition of 'foo'}}
 int __attribute__((target("arch=rv64gc"))) foo(void) { return 0; }
+
+//expected-warning@+1 {{unsupported 'notafeature' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("arch=+notafeature"))) UnsupportFeature(void) { return 0; }
+
+//expected-warning@+1 {{unsupported 'arch=+zba,zbb' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("arch=+zba,zbb"))) WithoutAddSigned(void) { return 0; }
+
+//expected-warning@+1 {{unsupported 'arch=zba' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("arch=zba"))) WithoutAddSigned2(void) { return 0; }

llvmbot · 2024-08-29T05:38:40Z

@llvm/pr-subscribers-clang

Author: Piyou Chen (BeMg)

Changes

…isValidFeatureName

This patch makes unsupported target attributes emit a warning and ignore the target attribute during semantic checks. The changes include:

Adding the RISCVTargetInfo::isValidFeatureName function.
Adding a '+' sign to __RISCV_TargetAttrNeedOverride because checkTargetAttr will remove the sign.
Rejecting non-full-arch strings in the handleFullArchString function.
Adding test cases to demonstrate the warning behavior.

Full diff: https://github.com/llvm/llvm-project/pull/106495.diff

4 Files Affected:

(modified) clang/lib/Basic/Targets/RISCV.cpp (+16-2)
(modified) clang/lib/Basic/Targets/RISCV.h (+1)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+11-4)
(modified) clang/test/Sema/attr-target-riscv.c (+9)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index 1f8a8cd1462c9d..1d6f49ca232692 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -257,7 +257,7 @@ bool RISCVTargetInfo::initFeatureMap(
 
   // If a target attribute specified a full arch string, override all the ISA
   // extension target features.
-  const auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
+  const auto I = llvm::find(FeaturesVec, "+__RISCV_TargetAttrNeedOverride");
   if (I != FeaturesVec.end()) {
     std::vector<std::string> OverrideFeatures(std::next(I), FeaturesVec.end());
 
@@ -391,7 +391,14 @@ void RISCVTargetInfo::fillValidTuneCPUList(
 
 static void handleFullArchString(StringRef FullArchStr,
                                  std::vector<std::string> &Features) {
-  Features.push_back("__RISCV_TargetAttrNeedOverride");
+
+  // Should be full arch string.
+  if (!FullArchStr.starts_with("rv")) {
+    Features.push_back(FullArchStr.str());
+    return;
+  }
+
+  Features.push_back("+__RISCV_TargetAttrNeedOverride");
   auto RII = llvm::RISCVISAInfo::parseArchString(
       FullArchStr, /* EnableExperimentalExtension */ true);
   if (llvm::errorToBool(RII.takeError())) {
@@ -485,3 +492,10 @@ bool RISCVTargetInfo::validateCpuSupports(StringRef Feature) const {
   // __riscv_feature_bits structure.
   return -1 != llvm::RISCVISAInfo::getRISCVFeaturesBitsInfo(Feature).second;
 }
+
+bool RISCVTargetInfo::isValidFeatureName(StringRef Name) const {
+  if (Name == "__RISCV_TargetAttrNeedOverride")
+    return true;
+
+  return llvm::RISCVISAInfo::isSupportedExtensionFeature(Name);
+}
diff --git a/clang/lib/Basic/Targets/RISCV.h b/clang/lib/Basic/Targets/RISCV.h
index 626274b8fc437c..b808ccc8e9cfe9 100644
--- a/clang/lib/Basic/Targets/RISCV.h
+++ b/clang/lib/Basic/Targets/RISCV.h
@@ -130,6 +130,7 @@ class RISCVTargetInfo : public TargetInfo {
   bool supportsCpuSupports() const override { return getTriple().isOSLinux(); }
   bool supportsCpuInit() const override { return getTriple().isOSLinux(); }
   bool validateCpuSupports(StringRef Feature) const override;
+  bool isValidFeatureName(StringRef Name) const override;
 };
 class LLVM_LIBRARY_VISIBILITY RISCV32TargetInfo : public RISCVTargetInfo {
 public:
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 1e074298ac5289..81cff8d7362ad5 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2993,10 +2993,17 @@ bool Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
            << Unknown << Tune << ParsedAttrs.Tune << Target;
 
-  if (Context.getTargetInfo().getTriple().isRISCV() &&
-      ParsedAttrs.Duplicate != "")
-    return Diag(LiteralLoc, diag::err_duplicate_target_attribute)
-           << Duplicate << None << ParsedAttrs.Duplicate << Target;
+  if (Context.getTargetInfo().getTriple().isRISCV()) {
+    if (ParsedAttrs.Duplicate != "")
+      return Diag(LiteralLoc, diag::err_duplicate_target_attribute)
+             << Duplicate << None << ParsedAttrs.Duplicate << Target;
+    for (const auto &Feature : ParsedAttrs.Features) {
+      auto CurFeature = StringRef(Feature);
+      if (!CurFeature.starts_with("+") && !CurFeature.starts_with("-"))
+        return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+               << Unsupported << None << AttrStr << Target;
+    }
+  }
 
   if (ParsedAttrs.Duplicate != "")
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
diff --git a/clang/test/Sema/attr-target-riscv.c b/clang/test/Sema/attr-target-riscv.c
index ed4e2915d6c6ef..01d928c1d78e48 100644
--- a/clang/test/Sema/attr-target-riscv.c
+++ b/clang/test/Sema/attr-target-riscv.c
@@ -4,3 +4,12 @@
 int __attribute__((target("arch=rv64g"))) foo(void) { return 0; }
 //expected-error@+1 {{redefinition of 'foo'}}
 int __attribute__((target("arch=rv64gc"))) foo(void) { return 0; }
+
+//expected-warning@+1 {{unsupported 'notafeature' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("arch=+notafeature"))) UnsupportFeature(void) { return 0; }
+
+//expected-warning@+1 {{unsupported 'arch=+zba,zbb' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("arch=+zba,zbb"))) WithoutAddSigned(void) { return 0; }
+
+//expected-warning@+1 {{unsupported 'arch=zba' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("arch=zba"))) WithoutAddSigned2(void) { return 0; }

clang/lib/Basic/Targets/RISCV.cpp

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Basic/Targets/RISCV.cpp

…reName

…start with "rv"

BeMg · 2024-08-31T12:16:41Z

Rebase with #106680 and drop the __RISCV_TargetAttrNeedOverride relate code section.

BeMg · 2024-09-05T05:19:23Z

ping

clang/test/Sema/attr-target-riscv.c

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/Sema/attr-target-riscv.c

topperc

LGTM

BeMg requested review from preames, lukel97, kito-cheng and topperc August 29, 2024 05:38

llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 29, 2024

kito-cheng reviewed Aug 29, 2024

View reviewed changes

clang/lib/Basic/Targets/RISCV.cpp Outdated Show resolved Hide resolved

dtcxzyw changed the title ~~[Clang][RISCV] Recognize unsupport target feature by supporting...~~ [Clang][RISCV] Recognize unsupport target feature by supporting isValidFeatureName Aug 29, 2024

topperc reviewed Aug 29, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

jrtc27 reviewed Aug 29, 2024

View reviewed changes

clang/lib/Basic/Targets/RISCV.cpp Outdated Show resolved Hide resolved

BeMg added 3 commits August 31, 2024 05:03

[Clang][RISCV] Recognize unsupport feature by supporting isValidFeatu…

e8f4726

…reName

Merge handleFullArchString logic, parseArchString will check whether …

ad95a31

…start with "rv"

use '+'/'-'

81d79a5

BeMg force-pushed the add-isValidFeatureName-for-RISCV branch from ef599d0 to 81d79a5 Compare August 31, 2024 12:10

Drop __RISCV_TargetAttrNeedOverride check

663de06

BeMg requested review from topperc, kito-cheng and jrtc27 August 31, 2024 12:16

michaelmaitland reviewed Sep 5, 2024

View reviewed changes

clang/test/Sema/attr-target-riscv.c Outdated Show resolved Hide resolved

topperc reviewed Sep 6, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/test/Sema/attr-target-riscv.c Outdated Show resolved Hide resolved

clang/test/Sema/attr-target-riscv.c Outdated Show resolved Hide resolved

BeMg added 3 commits September 5, 2024 21:50

AddSigned -> Plus

2b02795

Add -notafeature testcase

2d192be

Use StringRef replace auto

3b1f549

BeMg requested review from michaelmaitland and topperc September 6, 2024 05:15

topperc approved these changes Sep 6, 2024

View reviewed changes

BeMg merged commit 022b3c2 into llvm:main Sep 9, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][RISCV] Recognize unsupport target feature by supporting isValidFeatureName #106495

[Clang][RISCV] Recognize unsupport target feature by supporting isValidFeatureName #106495

Uh oh!

BeMg commented Aug 29, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 29, 2024

Uh oh!

llvmbot commented Aug 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BeMg commented Aug 31, 2024

Uh oh!

BeMg commented Sep 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

topperc left a comment

Uh oh!

Uh oh!

Uh oh!

[Clang][RISCV] Recognize unsupport target feature by supporting isValidFeatureName #106495

[Clang][RISCV] Recognize unsupport target feature by supporting isValidFeatureName #106495

Uh oh!

Conversation

BeMg commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 29, 2024

Uh oh!

llvmbot commented Aug 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BeMg commented Aug 31, 2024

Uh oh!

BeMg commented Sep 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BeMg commented Aug 29, 2024 •

edited

Loading