[Clang][ARM] Ensure FPU Features are parsed when targeting cc1as
#134612
Conversation
…Assembler Previously, FPU features were not collected when forming a list of features for the Assembler. This fixes a regression from 8fa0f0e, which caused VFPv4 to be unavailable if assembling for an armv7s-apple-darwin target. Co-authored-by: Martin Storsjö <[email protected]>
These changes were originally opened under llvm#134366 but were moved to this PR to allow me to drive further changes as I was the original author. This commit responds to comments made on the original PR.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-arm Author: Jack Styles (Stylie777) ChangesPreviously, With this change, all the features for the FPU will be passed to Co-authored-by: Martin Storsjö <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/134612.diff 6 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5217e04b5e83f..acbb2f435c864 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -38,6 +38,9 @@ Potentially Breaking Changes
- Fix missing diagnostics for uses of declarations when performing typename access,
such as when performing member access on a '[[deprecated]]' type alias.
(#GH58547)
+- For ARM targets, when using cc1as, the features included in the selected CPU or
+ Architecture's FPU are now loaded and utilized. If you wish not to use a specific feature,
+ the relevant ``+no`` option will need to be amended to the command line option.
C/C++ Language Potentially Breaking Changes
-------------------------------------------
@@ -463,6 +466,7 @@ X86 Support
Arm and AArch64 Support
^^^^^^^^^^^^^^^^^^^^^^^
+- For ARM targets, cc1as now considers the FPU's features for the selected CPU or Arch.
Android Support
^^^^^^^^^^^^^^^
diff --git a/clang/lib/Driver/ToolChains/Arch/ARM.cpp b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
index e50cb3836f2c9..ff0e52aa285b1 100644
--- a/clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -679,20 +679,17 @@ llvm::ARM::FPUKind arm::getARMTargetFeatures(const Driver &D,
CPUArgFPUKind != llvm::ARM::FK_INVALID ? CPUArgFPUKind : ArchArgFPUKind;
(void)llvm::ARM::getFPUFeatures(FPUKind, Features);
} else {
- bool Generic = true;
- if (!ForAS) {
- std::string CPU = arm::getARMTargetCPU(CPUName, ArchName, Triple);
- if (CPU != "generic")
- Generic = false;
- llvm::ARM::ArchKind ArchKind =
- arm::getLLVMArchKindForARM(CPU, ArchName, Triple);
- FPUKind = llvm::ARM::getDefaultFPU(CPU, ArchKind);
- (void)llvm::ARM::getFPUFeatures(FPUKind, Features);
- }
+ std::string CPU = arm::getARMTargetCPU(CPUName, ArchName, Triple);
+ bool Generic = CPU == "generic";
if (Generic && (Triple.isOSWindows() || Triple.isOSDarwin()) &&
getARMSubArchVersionNumber(Triple) >= 7) {
FPUKind = llvm::ARM::parseFPU("neon");
(void)llvm::ARM::getFPUFeatures(FPUKind, Features);
+ } else {
+ llvm::ARM::ArchKind ArchKind =
+ arm::getLLVMArchKindForARM(CPU, ArchName, Triple);
+ FPUKind = llvm::ARM::getDefaultFPU(CPU, ArchKind);
+ (void)llvm::ARM::getFPUFeatures(FPUKind, Features);
}
}
diff --git a/clang/test/Driver/arm-fpu-selection.s b/clang/test/Driver/arm-fpu-selection.s
new file mode 100644
index 0000000000000..c40e0f5b25852
--- /dev/null
+++ b/clang/test/Driver/arm-fpu-selection.s
@@ -0,0 +1,32 @@
+// REQUIRES: arm-registered-target
+// Ensures that when targeting an ARM target with an Asm file, clang
+// collects the features from the FPU. This is critical in the
+// activation of NEON for supported targets. The Cortex-R52 will be
+// used and tested for VFP and NEON Support
+
+// RUN: %clang -target arm-none-eabi -mcpu=cortex-r52 -c %s -o /dev/null | count 0
+// RUN: %clang -target arm-none-eabi -mcpu=cortex-r52 -c %s -o /dev/null -### 2>&1 | FileCheck --check-prefix=CHECK-TARGET-FEATURES %s
+
+// Check that NEON and VFPV5 have been activated when using Cortex-R52 when using cc1as
+// CHECK-TARGET-FEATURES: "-target-feature" "+vfp2sp"
+// CHECK-TARGET-FEATURES: "-target-feature" "+vfp3"
+// CHECK-TARGET-FEATURES: "-target-feature" "+fp-armv8"
+// CHECK-TARGET-FEATURES: "-target-feature" "+fp-armv8d16"
+// CHECK-TARGET-FEATURES: "-target-feature" "+fp-armv8d16sp"
+// CHECK-TARGET-FEATURES: "-target-feature" "+fp-armv8sp"
+// CHECK-TARGET-FEATURES: "-target-feature" "+neon"
+
+ vadd.f32 s0, s1, s2
+ vadd.f64 d0, d1, d2
+ vcvt.u32.f32 s0, s0, #1
+ vcvt.u32.f64 d0, d0, #1
+ vcvtb.f32.f16 s0, s1
+ vcvtb.f64.f16 d0, s1
+ vfma.f32 s0, s1, s2
+ vfma.f64 d0, d1, d2
+ vcvta.u32.f32 s0, s1
+ vcvta.u32.f64 s0, d1
+ vadd.f32 q0, q1, q2
+ vcvt.f32.f16 q0, d1
+ vfma.f32 q0, q1, q2
+ vcvta.u32.f32 q0, q1
diff --git a/clang/test/Driver/armv7-default-neon.s b/clang/test/Driver/armv7-default-neon.s
new file mode 100644
index 0000000000000..9caef97ce3b46
--- /dev/null
+++ b/clang/test/Driver/armv7-default-neon.s
@@ -0,0 +1,12 @@
+// Ensure that we can assemble NEON by just specifying an armv7
+// Apple or Windows target.
+
+// REQUIRES: arm-registered-target
+// RUN: %clang -c -target armv7-apple-darwin -o /dev/null %s
+// RUN: %clang -c -target armv7-apple-darwin -o /dev/null %s -### 2>&1 | FileCheck --check-prefix=CHECK-TARGET-FEATURES %s
+// RUN: %clang -c -target armv7-windows -o /dev/null %s
+// RUN: %clang -c -target armv7-windows -o /dev/null %s -### 2>&1 | FileCheck --check-prefix=CHECK-TARGET-FEATURES %s
+
+// CHECK-TARGET-FEATURES: "-target-feature" "+neon"
+
+vadd.i32 q0, q0, q0
diff --git a/clang/test/Driver/armv7s-default-vfpv4.s b/clang/test/Driver/armv7s-default-vfpv4.s
new file mode 100644
index 0000000000000..c9c8dbab9ae93
--- /dev/null
+++ b/clang/test/Driver/armv7s-default-vfpv4.s
@@ -0,0 +1,9 @@
+// Ensure that we can assemble VFPv4 by just specifying an armv7s target.
+
+// REQUIRES: arm-registered-target
+// RUN: %clang -c -target armv7s-apple-darwin -o /dev/null %s
+// RUN: %clang -c -target armv7s-apple-darwin -o /dev/null %s -### 2>&1 | FileCheck --check-prefix=CHECK-TARGET-FEATURES %s
+
+// CHECK-TARGET-FEATURES: "-target-feature" "+vfp4"
+
+vfma.f32 q1, q2, q3
diff --git a/clang/test/Driver/armv8.1m.main.s b/clang/test/Driver/armv8.1m.main.s
index 8fc94cf772fae..a660e56c2d1e7 100644
--- a/clang/test/Driver/armv8.1m.main.s
+++ b/clang/test/Driver/armv8.1m.main.s
@@ -8,21 +8,21 @@
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+fp -o /dev/null %s 2>%t
# RUN: FileCheck --check-prefix=ERROR-V81M_FP < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+nofp -o /dev/null %s 2>%t
-# RUN: FileCheck --check-prefix=ERROR-V81M_FP < %t %s
+# RUN: FileCheck --check-prefix=ERROR-V81M_NOFP < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+fp.dp -o /dev/null %s 2>%t
# RUN: FileCheck --check-prefix=ERROR-V81M_FPDP < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+nofp.dp -o /dev/null %s 2>%t
-# RUN: FileCheck --check-prefix=ERROR-V81M_FPDP < %t %s
+# RUN: FileCheck --check-prefix=ERROR-V81M_NOFPDP < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+mve -o /dev/null %s 2>%t
# RUN: FileCheck --check-prefix=ERROR-V81M_MVE < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+nomve -o /dev/null %s 2>%t
-# RUN: FileCheck --check-prefix=ERROR-V81M_MVE < %t %s
+# RUN: FileCheck --check-prefix=ERROR-V81M_NOMVE < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+mve+fp -o /dev/null %s 2>%t
# RUN: FileCheck --check-prefix=ERROR-V81M_MVE_FP < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+mve.fp -o /dev/null %s 2>%t
# RUN: FileCheck --check-prefix=ERROR-V81M_MVEFP < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+nomve.fp -o /dev/null %s 2>%t
-# RUN: FileCheck --check-prefix=ERROR-V81M_MVEFP < %t %s
+# RUN: FileCheck --check-prefix=ERROR-V81M_NOMVEFP < %t %s
.syntax unified
.thumb
@@ -35,28 +35,41 @@ qadd r0, r1, r2
# ERROR-V8M: :[[@LINE-1]]:1: error
# ERROR-V81M: :[[@LINE-2]]:1: error
# ERROR-V81M_FP: :[[@LINE-3]]:1: error
-# ERROR-V81M_FPDP: :[[@LINE-4]]:1: error
+# ERROR-V81M_NOFP: :[[@LINE-4]]:1: error
+# ERROR-V81M_FPDP: :[[@LINE-5]]:1: error
+# ERROR-V81M_NOFPDP: :[[@LINE-6]]:1: error
+# ERROR-V81M_NOMVE: :[[@LINE-7]]:1: error
+# ERROR-V81M_NOMVEFP: :[[@LINE-8]]:1: error
vadd.f16 s0, s1, s2
# ERROR-V8M: :[[@LINE-1]]:1: error
-# ERROR-V81M: :[[@LINE-2]]:1: error
-# ERROR-V81M_DSP: :[[@LINE-3]]:1: error
-# ERROR-V81M_MVE: :[[@LINE-4]]:1: error
+# ERROR-V81M_NOFP: :[[@LINE-2]]:1: error
vabs.f32 s0, s1
-# ERROR-V8M: :[[@LINE-1]]:1: error
-# ERROR-V81M: :[[@LINE-2]]:1: error
-# ERROR-V81M_DSP: :[[@LINE-3]]:1: error
-# ERROR-V81M_MVE: :[[@LINE-4]]:1: error
+# ERROR-V81M_NOFP: :[[@LINE-1]]:1: error
-vcmp.f64 d0,d1
+vabs.s32 q0, q1
# ERROR-V8M: :[[@LINE-1]]:1: error
# ERROR-V81M: :[[@LINE-2]]:1: error
# ERROR-V81M_DSP: :[[@LINE-3]]:1: error
# ERROR-V81M_FP: :[[@LINE-4]]:1: error
-# ERROR-V81M_MVE: :[[@LINE-5]]:1: error
-# ERROR-V81M_MVE_FP: :[[@LINE-6]]:1: error
-# ERROR-V81M_MVEFP: :[[@LINE-7]]:1: error
+# ERROR-V81M_NOFP: :[[@LINE-5]]:1: error
+# ERROR-V81M_FPDP: :[[@LINE-6]]:1: error
+# ERROR-V81M_NOFPDP: :[[@LINE-7]]:1: error
+# ERROR-V81M_NOMVE: :[[@LINE-8]]:1: error
+# ERROR-V81M_NOMVEFP: :[[@LINE-9]]:1: error
+
+vcmp.f64 d0,d1
+# ERROR-V81M: :[[@LINE-1]]:1: error
+# ERROR-V81M_DSP: :[[@LINE-2]]:1: error
+# ERROR-V81M_FP: :[[@LINE-3]]:1: error
+# ERROR-V81M_NOFP: :[[@LINE-4]]:1: error
+# ERROR-V81M_NOFPDP: :[[@LINE-5]]:1: error
+# ERROR-V81M_MVE: :[[@LINE-6]]:1: error
+# ERROR-V81M_NOMVE: :[[@LINE-7]]:1: error
+# ERROR-V81M_MVE_FP: :[[@LINE-8]]:1: error
+# ERROR-V81M_MVEFP: :[[@LINE-9]]:1: error
+# ERROR-V81M_NOMVEFP: :[[@LINE-10]]:1: error
asrl r0, r1, r2
# ERROR-V8M: :[[@LINE-1]]:1: error
@@ -64,6 +77,9 @@ asrl r0, r1, r2
# ERROR-V81M_DSP: :[[@LINE-3]]:1: error
# ERROR-V81M_FP: :[[@LINE-4]]:1: error
# ERROR-V81M_FPDP: :[[@LINE-5]]:1: error
+# ERROR-V81M_NOFPDP: :[[@LINE-6]]:1: error
+# ERROR-V81M_NOMVE: :[[@LINE-7]]:1: error
+# ERROR-V81M_NOMVEFP: :[[@LINE-8]]:1: error
vcadd.i8 q0, q1, q2, #90
# ERROR-V8M: :[[@LINE-1]]:1: error
@@ -71,3 +87,6 @@ vcadd.i8 q0, q1, q2, #90
# ERROR-V81M_DSP: :[[@LINE-3]]:1: error
# ERROR-V81M_FP: :[[@LINE-4]]:1: error
# ERROR-V81M_FPDP: :[[@LINE-5]]:1: error
+# ERROR-V81M_NOFPDP: :[[@LINE-6]]:1: error
+# ERROR-V81M_NOMVE: :[[@LINE-7]]:1: error
+# ERROR-V81M_NOMVEFP: :[[@LINE-8]]:1: error
|
Stylie777
changed the title
cc1ascc1as
Summary of changes - Adds check lines to ensure there are no errors/warnings - Makes all comments on test files `///` - Move the `getFPUFeatures` line so it is called outside of the if block that selects the FPU.
There was a problem hiding this comment.
Thanks for the review @DavidSpickett, I have made some updates based on this.
clang/docs/ReleaseNotes.rst
Outdated
| @@ -463,6 +466,7 @@ X86 Support | |||
|
|
|||
| Arm and AArch64 Support | |||
| ^^^^^^^^^^^^^^^^^^^^^^^ | |||
| - For ARM targets, cc1as now considers the FPU's features for the selected CPU or Arch. | |||
There was a problem hiding this comment.
Arch -> architecture
| if (Generic && (Triple.isOSWindows() || Triple.isOSDarwin()) && | ||
| getARMSubArchVersionNumber(Triple) >= 7) { | ||
| FPUKind = llvm::ARM::parseFPU("neon"); |
There was a problem hiding this comment.
Is the isOSWindows || isOSDarwin if still needed and does it solve the issues reported for darwin?
If the point of this code is to work around an upstream patch in ubuntu, I'm wondering if that is better to address in the upstream ubuntu patch. Otherwise we get into a tug of war between their patch and the one here, which doesn't sound like the best solution. getARMCPUForArch already has some cpu overrides for different OS's.
There was a problem hiding this comment.
Those are somewhat still needed in order to keep things working for that patched case, yes.
It's indeed a bit odd to avoid breakage for an out-of-tree downstream patch that isn't present here, this was discussed in #122095, but there @DavidSpickett agreed that it seems good to be explicit about these things - just like for the Android case just above here.
The problem with adjusting the downstream patch is that it is for Debian/Ubuntu packaging, adjusting what is included in the armv7 baseline (for the purposes of Linux targets), but affecting how useful it is as a cross compiler for other targets. But for them to take extra effort into their Linux-targeting patch to accommodate for using it for cross compilation for other OSes feels like a big thing to request anyway (even though it is their patch which is breaking things).
getARMCPUForArch already has some cpu overrides for different OS's.
Yeah, that would be possible as well. Windows does have one, and thanks to this patch it gets applied on both assembly and C. Darwin doesn't, but it should be possible to pick e.g. a cortex-a8 as baseline there.
There was a problem hiding this comment.
But for them to take extra effort into their Linux-targeting patch to accommodate for using it for cross compilation for other OSes feels like a big thing to request anyway (even though it is their patch which is breaking things).
Ubuntu includes other GCC cross-compiling toolchains in the packages they put out, so might be interested in allowing the cross-compiler to work for llvm if their patch can be adjusted. AFAIU they only want their change to apply to native linux compiles.
There was a problem hiding this comment.
Yeah that's definitely the intent of it - but making it work that way definitely is less obvious than one would want...
There was a problem hiding this comment.
The other thing this patch does is bring the preprocessor features for -march=armv8.1-m in line with the assembly features. It was previously enabling ARM_VFPV2 but not allowing vfpv2 instructions.
I'm still not sure about the Win/Darwin part, but otherwise LGTM.
We could fix it by adding a case for armv7 on Darwin in |
+1 to this, I agree this should be done in a separate patch. Thanks all for your reviews. |
…lvm#134612) Previously, `cc1as` did not consider the Features that can be included from a target's FPU. This could lead to a situation where assembly files could not compile as cc1as did not know if a feature was supported. With this change, all the features for the FPU will be passed to `cc1as` as `-target-feature` lines. By making this change, it will enable `+nosimd` to be functional, worked on in llvm#130623, and fix a regression introduced in 8fa0f0e so armv7s-apple-darwin targets can utilise VFPv4 correctly. --------- Co-authored-by: Martin Storsjö <[email protected]>
Previously,
cc1asdid not consider the Features that can be included from a target's FPU. This could lead to a situation where assembly files could not compile as cc1as did not know if a feature was supported.With this change, all the features for the FPU will be passed to
cc1asas-target-featurelines. By making this change, it will enable+nosimdto be functional, worked on in #130623, and fix a regression introduced in 8fa0f0e so armv7s-apple0darwin targets can utilise VFPv4 correctly.Co-authored-by: Martin Storsjö [email protected]