[clang] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc is specified in -target-feature (PR #132167)
https://github.com/XiaShark updated
https://github.com/llvm/llvm-project/pull/132167
>From 3be462100386014b3f78c1db5369487235a2bcf0 Mon Sep 17 00:00:00 2001
From: XiaShark
Date: Thu, 20 Mar 2025 14:32:30 +0800
Subject: [PATCH] [clang][AArch64] Don't define features macros when explicitly
disabled
Currently, clang has a general issue where it may incorrectly define
feature-specific macros(e.g., __ARM_FEATURE_CRC32) even when the
corresponding features are explicitly disabled via -target-feature. This
can lead to unexpected behavior in code that relies on these macros.
This commit fixed the issue by adding a secondary check to confirm if
the features are explicitly disabled in `-target-feature`.
---
clang/lib/Basic/Targets/AArch64.cpp | 171 +++---
.../Preprocessor/aarch64-target-features.c| 36
2 files changed, 144 insertions(+), 63 deletions(-)
diff --git a/clang/lib/Basic/Targets/AArch64.cpp
b/clang/lib/Basic/Targets/AArch64.cpp
index 3633bab6e0df9..98b91ebe77aab 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -914,6 +914,66 @@ void
AArch64TargetInfo::setFeatureEnabled(llvm::StringMap &Features,
bool AArch64TargetInfo::handleTargetFeatures(std::vector
&Features,
DiagnosticsEngine &Diags) {
+ // The first round, address the ARM version initially.
+ for (const auto &Feature : Features) {
+// All predecessor archs are added but select the latest one for ArchKind.
+if (Feature == "+v8a" && ArchInfo->Version < llvm::AArch64::ARMV8A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8A;
+if (Feature == "+v8.1a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_1A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_1A;
+if (Feature == "+v8.2a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_2A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_2A;
+if (Feature == "+v8.3a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_3A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_3A;
+if (Feature == "+v8.4a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_4A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_4A;
+if (Feature == "+v8.5a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_5A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_5A;
+if (Feature == "+v8.6a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_6A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_6A;
+if (Feature == "+v8.7a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_7A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_7A;
+if (Feature == "+v8.8a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_8A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_8A;
+if (Feature == "+v8.9a" &&
+ArchInfo->Version < llvm::AArch64::ARMV8_9A.Version)
+ ArchInfo = &llvm::AArch64::ARMV8_9A;
+if (Feature == "+v9a" && ArchInfo->Version < llvm::AArch64::ARMV9A.Version)
+ ArchInfo = &llvm::AArch64::ARMV9A;
+if (Feature == "+v9.1a" &&
+ArchInfo->Version < llvm::AArch64::ARMV9_1A.Version)
+ ArchInfo = &llvm::AArch64::ARMV9_1A;
+if (Feature == "+v9.2a" &&
+ArchInfo->Version < llvm::AArch64::ARMV9_2A.Version)
+ ArchInfo = &llvm::AArch64::ARMV9_2A;
+if (Feature == "+v9.3a" &&
+ArchInfo->Version < llvm::AArch64::ARMV9_3A.Version)
+ ArchInfo = &llvm::AArch64::ARMV9_3A;
+if (Feature == "+v9.4a" &&
+ArchInfo->Version < llvm::AArch64::ARMV9_4A.Version)
+ ArchInfo = &llvm::AArch64::ARMV9_4A;
+if (Feature == "+v9.5a" &&
+ArchInfo->Version < llvm::AArch64::ARMV9_5A.Version)
+ ArchInfo = &llvm::AArch64::ARMV9_5A;
+if (Feature == "+v9.6a" &&
+ArchInfo->Version < llvm::AArch64::ARMV9_6A.Version)
+ ArchInfo = &llvm::AArch64::ARMV9_6A;
+if (Feature == "+v8r")
+ ArchInfo = &llvm::AArch64::ARMV8R;
+ }
+
+ setDataLayout();
+ setArchFeatures();
+
+ // The second round, address each feature.
for (const auto &Feature : Features) {
if (Feature == "-fp-armv8")
HasNoFP = true;
@@ -944,6 +1004,8 @@ bool
AArch64TargetInfo::handleTargetFeatures(std::vector &Features,
HasFullFP16 = true;
HasSVE2 = true;
}
+if (Feature == "-sve2")
+ HasSVE2 = false;
if (Feature == "+sve2p1") {
FPU |= NeonMode;
FPU |= SveMode;
@@ -951,6 +1013,8 @@ bool
AArch64TargetInfo::handleTargetFeatures(std::vector &Features,
HasSVE2 = true;
HasSVE2p1 = true;
}
+if (Feature == "-sve2p1")
+ HasSVE2p1 = false;
if (Feature == "+sve-aes") {
FPU |= NeonMode;
HasFullFP16 = true;
@@ -994,12 +1058,16 @@ bool
AArch64TargetInfo::handleTargetFeatures(std::vector &Features,
HasBFloat16 = true;
HasFullFP16 = true;
}
+if (Feature == "-sme")
+ HasSME = false;
if (Feature == "+sme2") {
HasSME = true;
HasSME2 = true;
HasBFloat16 = true;
HasFullF
[clang] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc is specified in -target-feature (PR #132167)
XiaShark wrote: > > There are lots of bugs like this in this area, this is not the only feature > > like this. Should this be fixed in a more general way? > > We've looked at this internally but didn't get around to finishing it. > `HasD128` etc map to `Extensions` in `AArch64Features.td`, I think the way to > do it is to generate their declarations and initialisation code in > `ARMTargetDefEmitter.cpp` (`AArch64TargetParserDef.inc`). @XiaShark if you'd > like to look at it go ahead, otherwise let me know and I can put up a patch. I am currently attempting to add some conditional logic solely within the AArch64.cpp file to handle all the features. At present, it appears that the setArchFeatures() function forcibly enables certain features after acquiring the ArchInfo. Therefore, I am considering separating the judgment of ArchInfo from other features, placing them before and after the setArchFeatures() function call respectively, and processing each -feature. Could you advise if this approach would be effective? https://github.com/llvm/llvm-project/pull/132167 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc is specified in -target-feature (PR #132167)
tmatheson-arm wrote: > There are lots of bugs like this in this area, this is not the only feature > like this. Should this be fixed in a more general way? We've looked at this internally but didn't get around to finishing it. `HasD128` etc map to `Extensions` in `AArch64Features.td`, I think the way to do it is to generate their declarations and initialisation code in `ARMTargetDefEmitter.cpp` (`AArch64TargetParserDef.inc`). @XiaShark if you'd like to look at it go ahead, otherwise let me know and I can put up a patch. https://github.com/llvm/llvm-project/pull/132167 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc is specified in -target-feature (PR #132167)
lenary wrote: There are lots of bugs like this in this area, this is not the only feature like this. Should this be fixed in a more general way? https://github.com/llvm/llvm-project/pull/132167 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc is specified in -target-feature (PR #132167)
https://github.com/jthackray approved this pull request. LGTM. https://github.com/llvm/llvm-project/pull/132167 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc is specified in -target-feature (PR #132167)
llvmbot wrote:
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-aarch64
Author: None (XiaShark)
Changes
Currently, clang incorrectly defines the `__ARM_FEATURE_CRC32` macro even when
`-crc` is explicitly specified in `-target-feature`. This can lead to
unexpected behavior in code that relies on this macro.
See the example at https://godbolt.org/z/n4xsch3qv
This commit fixed the issue by adding a secondary check to confirm if `-crc` is
specified in `-target-feature`.
---
Full diff: https://github.com/llvm/llvm-project/pull/132167.diff
2 Files Affected:
- (modified) clang/lib/Basic/Targets/AArch64.cpp (+5-3)
- (modified) clang/test/Preprocessor/aarch64-target-features.c (+2)
``diff
diff --git a/clang/lib/Basic/Targets/AArch64.cpp
b/clang/lib/Basic/Targets/AArch64.cpp
index 3633bab6e0df9..f21e7d2c4d7b4 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1192,17 +1192,19 @@ bool
AArch64TargetInfo::handleTargetFeatures(std::vector &Features,
}
}
+ setDataLayout();
+ setArchFeatures();
+
// Check features that are manually disabled by command line options.
// This needs to be checked after architecture-related features are handled,
// making sure they are properly disabled when required.
for (const auto &Feature : Features) {
if (Feature == "-d128")
HasD128 = false;
+if (Feature == "-crc")
+ HasCRC = false;
}
- setDataLayout();
- setArchFeatures();
-
if (HasNoFP) {
FPU &= ~FPUMode;
FPU &= ~NeonMode;
diff --git a/clang/test/Preprocessor/aarch64-target-features.c
b/clang/test/Preprocessor/aarch64-target-features.c
index b10c55447d9af..6efad0f4983d7 100644
--- a/clang/test/Preprocessor/aarch64-target-features.c
+++ b/clang/test/Preprocessor/aarch64-target-features.c
@@ -120,7 +120,9 @@
// RUN: %clang -target arm64-none-linux-gnu -march=armv8-a+crc -x c -E -dM %s
-o - | FileCheck --check-prefix=CHECK-CRC32 %s
// RUN: %clang -target aarch64-none-linux-gnu -march=armv8.1-a -x c -E -dM %s
-o - | FileCheck --check-prefix=CHECK-CRC32 %s
// RUN: %clang -target arm64-none-linux-gnu -march=armv8.1-a -x c -E -dM %s -o
- | FileCheck --check-prefix=CHECK-CRC32 %s
+// RUN: %clang -target arm64-none-linux-gnu -march=armv8.1-a+nocrc -x c -E -dM
%s -o - | FileCheck --check-prefix=CHECK-NOCRC %s
// CHECK-CRC32: __ARM_FEATURE_CRC32 1
+// CHECK-NOCRC-NOT: __ARM_FEATURE_CRC32 1
// RUN: %clang -target aarch64-none-linux-gnu -fno-math-errno
-fno-signed-zeros\
// RUN:-fno-trapping-math -fassociative-math -freciprocal-math
-fapprox-func\
``
https://github.com/llvm/llvm-project/pull/132167
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc is specified in -target-feature (PR #132167)
https://github.com/XiaShark created
https://github.com/llvm/llvm-project/pull/132167
Currently, clang incorrectly defines the `__ARM_FEATURE_CRC32` macro even when
`-crc` is explicitly specified in `-target-feature`. This can lead to
unexpected behavior in code that relies on this macro.
See the example at https://godbolt.org/z/n4xsch3qv
This commit fixed the issue by adding a secondary check to confirm if `-crc` is
specified in `-target-feature`.
>From 8ad45b1c02dc762752b13eed91d9ed5bd60bbefd Mon Sep 17 00:00:00 2001
From: XiaShark
Date: Thu, 20 Mar 2025 14:32:30 +0800
Subject: [PATCH] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc
is specified in -target-feature
Currently, clang incorrectly defines the `__ARM_FEATURE_CRC32` macro
even when `-crc` is explicitly specified in `-target-feature`. This can
lead to unexpected behavior in code that relies on this macro.
This commit fixed the issue by adding a secondary check to confirm if
`-crc` is specified in `-target-feature`.
---
clang/lib/Basic/Targets/AArch64.cpp | 8 +---
clang/test/Preprocessor/aarch64-target-features.c | 2 ++
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/clang/lib/Basic/Targets/AArch64.cpp
b/clang/lib/Basic/Targets/AArch64.cpp
index 3633bab6e0df9..f21e7d2c4d7b4 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1192,17 +1192,19 @@ bool
AArch64TargetInfo::handleTargetFeatures(std::vector &Features,
}
}
+ setDataLayout();
+ setArchFeatures();
+
// Check features that are manually disabled by command line options.
// This needs to be checked after architecture-related features are handled,
// making sure they are properly disabled when required.
for (const auto &Feature : Features) {
if (Feature == "-d128")
HasD128 = false;
+if (Feature == "-crc")
+ HasCRC = false;
}
- setDataLayout();
- setArchFeatures();
-
if (HasNoFP) {
FPU &= ~FPUMode;
FPU &= ~NeonMode;
diff --git a/clang/test/Preprocessor/aarch64-target-features.c
b/clang/test/Preprocessor/aarch64-target-features.c
index b10c55447d9af..6efad0f4983d7 100644
--- a/clang/test/Preprocessor/aarch64-target-features.c
+++ b/clang/test/Preprocessor/aarch64-target-features.c
@@ -120,7 +120,9 @@
// RUN: %clang -target arm64-none-linux-gnu -march=armv8-a+crc -x c -E -dM %s
-o - | FileCheck --check-prefix=CHECK-CRC32 %s
// RUN: %clang -target aarch64-none-linux-gnu -march=armv8.1-a -x c -E -dM %s
-o - | FileCheck --check-prefix=CHECK-CRC32 %s
// RUN: %clang -target arm64-none-linux-gnu -march=armv8.1-a -x c -E -dM %s -o
- | FileCheck --check-prefix=CHECK-CRC32 %s
+// RUN: %clang -target arm64-none-linux-gnu -march=armv8.1-a+nocrc -x c -E -dM
%s -o - | FileCheck --check-prefix=CHECK-NOCRC %s
// CHECK-CRC32: __ARM_FEATURE_CRC32 1
+// CHECK-NOCRC-NOT: __ARM_FEATURE_CRC32 1
// RUN: %clang -target aarch64-none-linux-gnu -fno-math-errno
-fno-signed-zeros\
// RUN:-fno-trapping-math -fassociative-math -freciprocal-math
-fapprox-func\
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][AArch64] Don't #define __ARM_FEATURE_CRC32 when -crc is specified in -target-feature (PR #132167)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/132167 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
