[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
https://github.com/preames closed https://github.com/llvm/llvm-project/pull/74889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
preames wrote: > 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. https://github.com/llvm/llvm-project/pull/74889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
https://github.com/MaskRay approved this pull request. https://github.com/llvm/llvm-project/pull/74889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
topperc wrote: > > 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. https://github.com/llvm/llvm-project/pull/74889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
BeMg wrote: > 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. https://github.com/llvm/llvm-project/pull/74889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
https://github.com/BeMg approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/74889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
https://github.com/topperc approved this pull request. 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. https://github.com/llvm/llvm-project/pull/74889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
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
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 : 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);
}
``
https://github.com/llvm/llvm-project/pull/74889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
llvmbot wrote:
@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
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 ImpliedFeatures = (*ParseResult)->toFeatureVector();
- // Add non-ISA features like `relax` and `save-restore` back
- for (const std::string : 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 : 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"
[clang] [RISCV] Enable target attribute when invoked through clang driver (PR #74889)
https://github.com/preames created
https://github.com/llvm/llvm-project/pull/74889
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.
>From 65707b837a8bb7283896d2c9d4933a17e02a20b9 Mon Sep 17 00:00:00 2001
From: Philip Reames
Date: Fri, 8 Dec 2023 12:49:58 -0800
Subject: [PATCH] [RISCV] Enable target attribute when invoked through clang
driver
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.
---
clang/lib/Basic/Targets/RISCV.cpp | 15 +
.../CodeGen/RISCV/riscv-func-attr-target.c| 21 ++-
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/clang/lib/Basic/Targets/RISCV.cpp
b/clang/lib/Basic/Targets/RISCV.cpp
index 13f934e9947212..184176c05da23b 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 ImpliedFeatures = (*ParseResult)->toFeatureVector();
- // Add non-ISA features like `relax` and `save-restore` back
- for (const std::string : 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 : 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 74bc5f2ac70492..506acaba687417 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"
