Skip to content

[ARM] Emit a warning when the hard-float ABI is enabled but can't be used. #111334

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[ARM] Emit a warning when the hard-float ABI is enabled but can't be used. #111334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 1 deletion llvm/lib/Target/ARM/ARMISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,9 @@ ARMTargetLowering::ARMTargetLowering(const TargetMachine &TM,
setAllExpand(MVT::f32);
if (!Subtarget->hasFP64())
setAllExpand(MVT::f64);
}
} else if (TM.Options.FloatABIType == FloatABI::Hard)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I run this on a test program I get a couple of warnings

bin/clang --target=arm-none-eabihf -march=armv7-r nofp.c -c -mfloat-abi=hard -ffunction-sections
clang: warning: '-mfloat-abi=hard': selected processor lacks floating point registers [-Wunsupported-abi]
The hard-float ABI can't be used for a target that doesn't support floating-point (ignoring float-abi)
The hard-float ABI can't be used for a target that doesn't support floating-point (ignoring float-abi)

I think this is because this constructor is run once per ARMSubtarget. Tracing this back to getSubtargetImpl where ARMSubtargets are created https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/ARM/ARMTargetMachine.cpp#L280

It looks like that might be a better place to put this diagnostic. We may also want to see if some additional information like the function name can be used, possibly something from the Subtarget if the output is human readable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That does look like a better place for this, and already has one check like this, emitted through the LLVMContext so that the compiler can exit with a non-zero status code, or report the error through whatever other means it normally used. I think an error is better than a warning for this, because it's not possible to generate the requested code correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first version of the PR did put the check in that spot, but it leads to several required test changes because many tests invoke llc or lto with hardfp and no fp support, but then don't actually use fp, whereas the current patch does not result in those tests failing. Moving the check there means that those tests will need to be changed to invoke the LLVM tools with correct options. What is the reviewers' preference? @ostannard @smithp35

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be best to update those tests, probably to use the the soft-float ABI if they don't actually use floating-point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm happy to follow @ostannard 's choice here. Fixing up tests to specify the correct flags is a good thing.

My concern last year, apologies I've forgotten a lot of the details, was if these represent any use cases we may encounter with clang, possibly using some target attributes or with LTO. However after doing our best to rule this out, we can give it a go and see if we hit anything we didn't think about beforehand.

Copy link
Contributor Author

@chrisnc chrisnc Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The specific case of concern was this one, which is still ultimately due to not requesting the correct features at link time, and so the tool has to use the features the linker was invoked with if it needs to create a new function that didn't exist in the input. I don't see an obvious way around that problem. I would contend that users should be invoking their linker with the correct settings to avoid issues like this in general, so in some sense it exposes a potential problem in such builds anyway.
#111334 (comment)

errs() << "The hard-float ABI can't be used for a target that "
"doesn't support floating-point (ignoring float-abi)\n";

if (Subtarget->hasFullFP16()) {
addRegisterClass(MVT::f16, &ARM::HPRRegClass);
Expand Down
15 changes: 15 additions & 0 deletions llvm/test/CodeGen/ARM/eabihf-no-fpregs.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
; RUN: llc -asm-verbose=false --mtriple=armv7-none-eabihf --mattr=+vfp3 < %s | FileCheck %s --check-prefix=CHECK-VFP
; RUN: llc -asm-verbose=false --mtriple=armv7-none-eabi --mattr=-fpregs < %s | FileCheck %s -check-prefix=CHECK-NOVFP
; RUN: llc -asm-verbose=false --mtriple=armv7-none-eabihf --mattr=-fpregs < %s 2>&1 | FileCheck %s -check-prefix=CHECK-ERROR -check-prefix=CHECK-NOVFP

target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-n32"

; CHECK-VFP: vadd.f32
; CHECK-ERROR: The hard-float ABI can't be used for a target that doesn't support floating-point (ignoring float-abi)
; CHECK-NOVFP: bl __aeabi_fadd
define float @test_fadd(float %a, float %b) #0 {
%r = fadd float %a, %b
ret float %r
}

attributes #0 = { nounwind }