[RISCV] Enable target attribute when invoked through clang driver #74889

preames · 2023-12-08T21:28:05Z

d80e46d added support for the target function attribute. However, it turns out that commit has a nasty bug/oversight. As the tests in that revision show, everything works if clang -cc1 is directly invoked. I was suprised to learn this morning that compiling with clang (i.e. the typical user workflow) did not work.

The bug is that if a set of explicit negative extensions is passed to cc1 at the command line (as the clang driver always does), we were copying these negative extensions to the end of the rewritten extension list. When this was later parsed, this had the effect of turning back off any extension that the target attribute had enabled.

This patch updates the logic to only propagate the features from the input which don't appear in the rewritten form in either positive or negative form.

Note that this code structure is still highly suspect. In particular I'm fairly sure that mixing extension versions with this code will result in odd results. However, I figure its better to have something which mostly works than something which doesn't work at all.

d80e46d added support for the target function attribute. However, it turns out that commit has a nasty bug/oversight. As the tests in that revision show, everything works if clang -cc1 is directly invoked. I was suprised to learn this morning that compiling with clang (i.e. the typical user workflow) did not work. The bug is that if a set of explicit negative extensions is passed to cc1 at the command line (as the clang driver always does), we were copying these negative extensions to the end of the rewritten extension list. When this was later parsed, this had the effect of turning back off any extension that the target attribute had enabled. This patch updates the logic to only propagate the features from the input which don't appear in the rewritten form in either positive or negative form. Note that this code structure is still highly suspect. In particular I'm fairly sure that mixing extension versions with this code will result in odd results. However, I figure its better to have something which mostly works than something which doesn't work at all.

llvmbot · 2023-12-08T21:28:31Z

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

@llvm/pr-subscribers-clang

Author: Philip Reames (preames)

Changes

d80e46d added support for the target function attribute. However, it turns out that commit has a nasty bug/oversight. As the tests in that revision show, everything works if clang -cc1 is directly invoked. I was suprised to learn this morning that compiling with clang (i.e. the typical user workflow) did not work.

The bug is that if a set of explicit negative extensions is passed to cc1 at the command line (as the clang driver always does), we were copying these negative extensions to the end of the rewritten extension list. When this was later parsed, this had the effect of turning back off any extension that the target attribute had enabled.

This patch updates the logic to only propagate the features from the input which don't appear in the rewritten form in either positive or negative form.

Note that this code structure is still highly suspect. In particular I'm fairly sure that mixing extension versions with this code will result in odd results. However, I figure its better to have something which mostly works than something which doesn't work at all.

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

2 Files Affected:

(modified) clang/lib/Basic/Targets/RISCV.cpp (+11-4)
(modified) clang/test/CodeGen/RISCV/riscv-func-attr-target.c (+11-10)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index 13f934e994721..184176c05da23 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -304,11 +304,18 @@ bool RISCVTargetInfo::initFeatureMap(
 
   // RISCVISAInfo makes implications for ISA features
   std::vector<std::string> ImpliedFeatures = (*ParseResult)->toFeatureVector();
-  // Add non-ISA features like `relax` and `save-restore` back
-  for (const std::string &Feature : NewFeaturesVec)
-    if (!llvm::is_contained(ImpliedFeatures, Feature))
-      ImpliedFeatures.push_back(Feature);
 
+  // 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);
 }
 
diff --git a/clang/test/CodeGen/RISCV/riscv-func-attr-target.c b/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
index 74bc5f2ac7049..506acaba68741 100644
--- a/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
+++ b/clang/test/CodeGen/RISCV/riscv-func-attr-target.c
@@ -1,6 +1,7 @@
 // REQUIRES: riscv-registered-target
 // RUN: %clang_cc1 -triple riscv64 -target-feature +zifencei -target-feature +m \
-// RUN:  -target-feature +a -target-feature +save-restore \
+// RUN:  -target-feature +a -target-feature +save-restore -target-feature -zbb \
+// RUN:  -target-feature -relax -target-feature -zfa \
 // RUN:  -emit-llvm %s -o - | FileCheck %s
 
 // CHECK-LABEL: define dso_local void @testDefault
@@ -35,12 +36,12 @@ testAttrFullArchAndAttrCpu() {}
 __attribute__((target("cpu=sifive-u54"))) void testAttrCpuOnly() {}
 
 //.
-// CHECK: attributes #0 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zifencei" }
-// 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" "tune-cpu"="generic-rv64" }
-// CHECK: attributes #2 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei" }
-// 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" }
-// CHECK: attributes #4 = { {{.*}}"target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zbb,+zicsr,+zifencei" }
-// CHECK: attributes #5 = { {{.*}}"target-features"="+64bit,+m,+save-restore" }
-// CHECK: attributes #6 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei" }
-// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+m,+save-restore" }
-// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei" }
+// CHECK: attributes #0 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zifencei,-relax,-zbb,-zfa" }
+// 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,-zfa" }
+// CHECK: attributes #5 = { {{.*}}"target-features"="+64bit,+m,+save-restore,-relax,-zbb,-zfa" }
+// 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,-zbb,-zfa" }
+// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,-relax,-zbb,-zfa" }

github-actions · 2023-12-08T21:30:38Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 05420a17547e495f5748e9662150d6eb931e2c28 65707b837a8bb7283896d2c9d4933a17e02a20b9 -- clang/lib/Basic/Targets/RISCV.cpp clang/test/CodeGen/RISCV/riscv-func-attr-target.c

View the diff from clang-format here.

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index 184176c05d..1cce8fe5b2 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -309,12 +309,12 @@ bool RISCVTargetInfo::initFeatureMap(
   // 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);
+    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);
 }

topperc

LGTM

Related question. If there is an -mcpu on the command line and target attribute changes the march, do we keep the original CPU in the -target-cpu attribute or drop it. The reason for all those negative features from the driver was to make the backend not infer any features from the CPU that weren't in the provided march. So I'm wondering if we have that issue with the target attribute now.

BeMg

LGTM

BeMg · 2023-12-11T03:35:56Z

LGTM

Related question. If there is an -mcpu on the command line and target attribute changes the march, do we keep the original CPU in the -target-cpu attribute or drop it. The reason for all those negative features from the driver was to make the backend not infer any features from the CPU that weren't in the provided march. So I'm wondering if we have that issue with the target attribute now.

Compiler will keep target-cpu from command line when target attribute doesn't assign new cpu option.

topperc · 2023-12-11T04:07:18Z

LGTM

Related question. If there is an -mcpu on the command line and target attribute changes the march, do we keep the original CPU in the -target-cpu attribute or drop it. The reason for all those negative features from the driver was to make the backend not infer any features from the CPU that weren't in the provided march. So I'm wondering if we have that issue with the target attribute now.

Compiler will keep target-cpu from command line when target attribute doesn't assign new cpu option.

So that's a bug then. If the command line CPU has vector, but the target attribute doesn't. The backend will still use vector instructions.

preames · 2023-12-11T16:54:23Z

Related question. If there is an -mcpu on the command line and target attribute changes the march, do we keep the original CPU in the -target-cpu attribute or drop it. The reason for all those negative features from the driver was to make the backend not infer any features from the CPU that weren't in the provided march. So I'm wondering if we have that issue with the target attribute now.

It looks like the original -mcpu from the command line is passed through the to the function attributes unless the cpu is overridden via the target attribute. Doing a full replacement of the march does not appear to change or remove the target-cpu attribute on the result.

One case I could see being problematic would be a CPU which enabled e.g. V, and a replacement march which didn't.

Before this change, we'd end up with with an explicit target-feature for +v (coming from the original list passed into cc1), and that's clearly wrong. After this change, we end up with the explicit features being entirely missing (i.e. no explicit +v).

I had to go run a test to see what happened from there. It does look like we re-infer from the cpu definition and proceed to generate vector code. Yeah, that's more than a bit suspect.

This does fall into the "extension subtraction" case we'd tried to leave out of the specification. So I can see two fixes here: add back the explicit negative feature, or update the specification text to disallow this.

…attribute This patch reworks RISCVTargetInfo::initFeatureMap to fix the issue described in llvm#74889 (review) (and is an alternative to llvm#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 llvm#74889.

…attribute (#77426) This patch reworks RISCVTargetInfo::initFeatureMap to fix the issue described in #74889 (review) (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.

preames requested review from asb, kito-cheng, BeMg and topperc December 8, 2023 21:28

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 Dec 8, 2023

topperc approved these changes Dec 9, 2023

View reviewed changes

BeMg approved these changes Dec 11, 2023

View reviewed changes

MaskRay approved these changes Dec 11, 2023

View reviewed changes

preames merged commit 99c0a3e into llvm:main Dec 11, 2023

preames deleted the clang-riscv-target-attribute-via-driver branch December 11, 2023 16:55

BeMg mentioned this pull request Dec 18, 2023

[RISCV] Update TargetAttr target-cpu override rule #75804

Closed

lukel97 mentioned this pull request Jan 4, 2024

[RISCV] Fix collectNonISAExtFeature returning negative extension features #76962

Merged

lukel97 mentioned this pull request Jan 9, 2024

[RISCV] Overwrite cpu target features for full arch string in target attribute #77426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RISCV] Enable target attribute when invoked through clang driver #74889

[RISCV] Enable target attribute when invoked through clang driver #74889

Uh oh!

preames commented Dec 8, 2023

Uh oh!

llvmbot commented Dec 8, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Dec 8, 2023

Uh oh!

topperc left a comment

Uh oh!

BeMg left a comment

Uh oh!

BeMg commented Dec 11, 2023

Uh oh!

topperc commented Dec 11, 2023

Uh oh!

preames commented Dec 11, 2023

Uh oh!

Uh oh!

[RISCV] Enable target attribute when invoked through clang driver #74889

[RISCV] Enable target attribute when invoked through clang driver #74889

Uh oh!

Conversation

preames commented Dec 8, 2023

Uh oh!

llvmbot commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 8, 2023

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

BeMg left a comment

Choose a reason for hiding this comment

Uh oh!

BeMg commented Dec 11, 2023

Uh oh!

topperc commented Dec 11, 2023

Uh oh!

preames commented Dec 11, 2023

Uh oh!

Uh oh!

llvmbot commented Dec 8, 2023 •

edited

Loading