Skip to content

[Clang][ARM] Ensure FPU Features are parsed when targeting cc1as #134612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 14, 2025
Merged

[Clang][ARM] Ensure FPU Features are parsed when targeting cc1as #134612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5b81d2
[Clang] [ARM] Ensure FPU Features are collected when using the Clang …
Stylie777 Mar 7, 2025
e6e916f
Respond to review comments originally from #134366
Stylie777 Apr 7, 2025
7861b31
Respond to review comments
Stylie777 Apr 10, 2025
ecefc66
Fix spelling in Release Notes
Stylie777 Apr 10, 2025
0e88318
Improve release notes based on review comments.
Stylie777 Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 compiling assembly files, the features included in the selected CPU
or Architecture's FPU are included. 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
-------------------------------------------
Expand Down Expand Up @@ -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 Architecture.

Android Support
^^^^^^^^^^^^^^^
Expand Down
18 changes: 7 additions & 11 deletions clang/lib/Driver/ToolChains/Arch/ARM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,21 +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");
Comment on lines 684 to 686
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's definitely the intent of it - but making it work that way definitely is less obvious than one would want...

(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);
}

// Now we've finished accumulating features from arch, cpu and fpu,
Expand Down
36 changes: 36 additions & 0 deletions clang/test/Driver/arm-fpu-selection.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// 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 2>&1 | FileCheck --check-prefix=CHECK-STDERR %s --allow-empty
// 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 no errors or warnings are present when assembling using cc1as.
// CHECK-STDERR-NOT: error:
// CHECK-STDERR-NOT: warning:

/// 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
16 changes: 16 additions & 0 deletions clang/test/Driver/armv7-default-neon.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/// 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 2>&1 | FileCheck --check-prefix=CHECK-STDERR %s --allow-empty
// 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 2>&1 | FileCheck --check-prefix=CHECK-STDERR %s --allow-empty
// RUN: %clang -c -target armv7-windows -o /dev/null %s -### 2>&1 | FileCheck --check-prefix=CHECK-TARGET-FEATURES %s

/// Check that no errors or warnings are present when assembling using cc1as.
// CHECK-STDERR-NOT: error:
// CHECK-STDERR-NOT: warning:

// CHECK-TARGET-FEATURES: "-target-feature" "+neon"

vadd.i32 q0, q0, q0
13 changes: 13 additions & 0 deletions clang/test/Driver/armv7s-default-vfpv4.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/// 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 2>&1 | FileCheck --check-prefix=CHECK-STDERR %s --allow-empty
// RUN: %clang -c -target armv7s-apple-darwin -o /dev/null %s -### 2>&1 | FileCheck --check-prefix=CHECK-TARGET-FEATURES %s

/// Check that no errors or warnings are present when assembling using cc1as.
// CHECK-STDERR-NOT: error:
// CHECK-STDERR-NOT: warning:

// CHECK-TARGET-FEATURES: "-target-feature" "+vfp4"

vfma.f32 q1, q2, q3
51 changes: 35 additions & 16 deletions clang/test/Driver/armv8.1m.main.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -35,39 +35,58 @@ 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
# ERROR-V81M: :[[@LINE-2]]:1: error
# 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
# ERROR-V81M: :[[@LINE-2]]:1: error
# 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
Loading