[builtins][AArch32] Fix __gnu_* functions #137638

saturn691 · 2025-04-28T14:28:17Z

Move to a consistent calling convention for both Clang/GNU such that they can be linked with each other.

All ARM targets now use the soft-float calling convention for __gnu_h2f_ieee and __gnu_f2h_ieee, as described in https://github.com/ARM-software/abi-aa/blob/main/rtabi32/rtabi32.rst#the-floating-point-helper-functions.

Move to a consistent calling convention for both Clang and GNU such that they can be linked with each other.

github-actions · 2025-04-28T14:28:39Z

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.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

smithp35 · 2025-04-30T13:52:01Z

I think we may need a bit more evidence collected to make sure we're doing the right thing across all platforms.

When I look at the ARM libgcc prototypes https://github.com/gcc-mirror/gcc/blob/master/libgcc/config/arm/fp16.c#L56

unsigned short __gnu_f2h_ieee (unsigned int a);
unsigned int __gnu_h2f_ieee (unsigned short a);
unsigned short __gnu_f2h_alternative (unsigned int x);
unsigned int __gnu_h2f_alternative (unsigned short a);
unsigned short __gnu_d2h_ieee (unsigned long long a);
unsigned short __gnu_d2h_alternative (unsigned long long x);

These have a core register interface, and I'm not sure your proposed change reflects that for the source parameter. As it looks like src_t may be float.

So if an Arm GCC compiled object calls one of these functions I think it will still be wrong.

Looking at LLVM ARMISelLowering.cpp these functions may be called by MachO

  } else if (!Subtarget->isTargetMachO()) {
    setLibcallName(RTLIB::FPROUND_F32_F16, "__gnu_f2h_ieee");
    setLibcallName(RTLIB::FPEXT_F16_F32, "__gnu_h2f_ieee");
  }

It is highly likely that the interface is correct for MachO but not for calls via libgcc.

Can you do some research to make sure:

What targets in GCC may call __gnu_h2f and friends and what is their expected interface?
What targets in LLVM may call __gnu_h2f and friends and what is their expected interface?
Can you make sure that the change to compiler-rt works for all of these targets. For example we may need to compile differently for MachO (Darwin) than for ELF.

saturn691 · 2025-04-30T14:25:32Z

What targets in GCC may call __gnu_h2f and friends and what is their expected interface?

float h2f(_Float16 x)
{
    return (float)x;
}

int main()
{
    float x = h2f(1.0f16);
    return !(x == 1.0f);
}

This is the test I am running.
From looking at the new generated assembly, it seems we are using the correct interface (u16 -> u32) as defined in libgcc.

200000ec <__gnu_h2f_ieee>:
200000ec: e92d4800     	push	{r11, lr}
200000f0: ee000a10     	vmov	s0, r0
200000f4: ebffffe1     	bl	0x20000080 <__extendhfsf2> @ imm = #-0x7c
200000f8: ee100a10     	vmov	r0, s0
200000fc: e8bd8800     	pop	{r11, pc}

20000100 <__aeabi_h2f>:
20000100: e92d4800     	push	{r11, lr}
20000104: ee000a10     	vmov	s0, r0
20000108: ebffffdc     	bl	0x20000080 <__extendhfsf2> @ imm = #-0x90
2000010c: ee100a10     	vmov	r0, s0
20000110: e8bd8800     	pop	{r11, pc}

What targets in LLVM may call __gnu_h2f and friends and what is their expected interface?

LLVM uses __aeabi_h2f instead. Hence why this change is here to standardise the interfaces.

Can you make sure that the change to compiler-rt works for all of these targets. For example we may need to compile differently for MachO (Darwin) than for ELF.

I've tested it on Linux- it works. It's actually quite hard to write a test case that works for this without inline assembly. Also, in the .cpp - it says if Subtarget is not (!) MachO. This change I presume does not affect MachO targets.

smithp35 · 2025-04-30T15:38:39Z

Apologies for taking so long to reply, I wanted to track down the history to make sure that this isn't going to break any none Arm target, the code as it stands shouldn't have worked at all for Arm targets, but historically the __gnu_h2f_ieee used to be the default lowering for all targets (including non-Arm). The original commit was added in https://reviews.llvm.org/D9693 later the default lowering was changed in #126880 to go to __truncsfhf2 so this won't affect other targets.

Apologies again for missing the not on the condition for machO, looking at the full code and the test at https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/ARM/fp16.ll it seems like the gnueabi musleabi are what selects the GNU libcall from LLVM.

From clang it is possible to generate a call with clang -march=armv7-a+fp16 -mfloat-abi=hard --target=arm-none-gnueabihf -S -o - h2f.c -O1. If armv8-a is used there are inline instructions used instead.

h2f:
        .fnstart
@ %bb.0:                                @ %entry
        .save   {r11, lr}
        push    {r11, lr}
        .setfp  r11, sp
        mov     r11, sp
        vmov    r0, s0
        bl      __gnu_h2f_ieee
        vmov    s0, r0
        pop     {r11, pc}
.Lfunc_end0:

Looking up what AEABI_RTABI expands to

compiler-rt/lib/builtins/int_lib.h:#define AEABI_RTABI __attribute__((__pcs__("aapcs")))

That will cause the function to use the soft-float calling convention regardless of the parameters so this should be compatible with the GCC prototypes regardless of prototype.

To me, it seems weird that we are using r0 since it has to be moved to s0 to be used with __extendhfsf2. I suspect there may have been some issues in the past for it to be purposely written this way, as it takes extra instructions for both conversions.

It is a property of the ABI of the helper functions. In Arm the base-standard procedure call standard passes floating point values in core-registers, there is a hard-float variant that passes floating point values in floating point registers. The Arm ABI defines that the helper functions to always use the base standard (https://github.com/ARM-software/abi-aa/blob/main/rtabi32/rtabi32.rst#410__hardfp_-name-mangling). The __extendhfsf2 function is not part of the Arm ABI so it has the hard-float ABI

smithp35

Your description will become the commit message (can be altered at squash and merge time).

Will be worth rewriting it to be closer to the intended commit message if you don't have commit access.

Will be worth mentioning that the __gnu_h2f_ieee and __gnu_f2h_ieee functions in Arm use the base-standard ABI like the __aeabi_ functions so we must use a call for hard-float Arm targets but can use an alias for base-standard targets.

smithp35 · 2025-04-30T16:15:32Z

compiler-rt/lib/builtins/extendhfsf2.c

@@ -16,12 +16,12 @@ COMPILER_RT_ABI NOINLINE float __extendhfsf2(src_t a) {
  return __extendXfYf2__(a);
 }

-COMPILER_RT_ABI float __gnu_h2f_ieee(src_t a) { return __extendhfsf2(a); }


This is the default for all non-Arm targets, as written this will disappear from all but the __ARM_EABI__ targets.

This is probably fine in practice as at least recent versions of clang will not call __gnu_h2f_ieee and __gnu_f2h_ieee for non Arm targets. It will be worth a #else for #if defined(__ARM_EABI__) with this line just in case a new compiler-rt is used with an old object with such a call.

yep it is below

saturn691 · 2025-04-30T16:30:11Z

Ok, commit is ready to merge at any time.

smithp35

Approved on my side. Will leave the PR open for a few days to give other reviewers a chance to comment

compiler-rt/lib/builtins/extendhfsf2.c

github-actions · 2025-05-19T10:07:07Z

✅ With the latest revision this PR passed the C/C++ code formatter.

statham-arm

That looks better, thanks. LGTM.

github-actions · 2025-05-19T12:09:34Z

@saturn691 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

[builtins][AArch64] Fix __gnu_* functions

feabb90

Move to a consistent calling convention for both Clang and GNU such that they can be linked with each other.

llvmbot added compiler-rt compiler-rt:builtins labels Apr 28, 2025

saturn691 changed the title ~~[builtins][AArch64] Fix __gnu_* functions~~ [builtins][AArch32] Fix __gnu_* functions Apr 30, 2025

fixup! [builtins][AArch64] Fix __gnu_* functions

29b6431

smithp35 reviewed Apr 30, 2025

View reviewed changes

smithp35 approved these changes Apr 30, 2025

View reviewed changes

statham-arm reviewed May 16, 2025

View reviewed changes

compiler-rt/lib/builtins/extendhfsf2.c Show resolved Hide resolved

fixup! fixup! [builtins][AArch64] Fix __gnu_* functions

0632b79

fixup! fixup! fixup! [builtins][AArch64] Fix __gnu_* functions

345e0b6

statham-arm approved these changes May 19, 2025

View reviewed changes

statham-arm merged commit e8a3074 into llvm:main May 19, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[builtins][AArch32] Fix __gnu_* functions #137638

[builtins][AArch32] Fix __gnu_* functions #137638

Uh oh!

saturn691 commented Apr 28, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 28, 2025

Uh oh!

smithp35 commented Apr 30, 2025

Uh oh!

saturn691 commented Apr 30, 2025

Uh oh!

smithp35 commented Apr 30, 2025

Uh oh!

smithp35 left a comment

Uh oh!

smithp35 Apr 30, 2025

Uh oh!

saturn691 Apr 30, 2025

Uh oh!

saturn691 commented Apr 30, 2025

Uh oh!

smithp35 left a comment

Uh oh!

Uh oh!

github-actions bot commented May 19, 2025 •

edited

Loading

Uh oh!

statham-arm left a comment

Uh oh!

Uh oh!

github-actions bot commented May 19, 2025

Uh oh!

Uh oh!

[builtins][AArch32] Fix __gnu_* functions #137638

[builtins][AArch32] Fix __gnu_* functions #137638

Uh oh!

Conversation

saturn691 commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 28, 2025

Uh oh!

smithp35 commented Apr 30, 2025

Uh oh!

saturn691 commented Apr 30, 2025

Uh oh!

smithp35 commented Apr 30, 2025

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

saturn691 Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

saturn691 commented Apr 30, 2025

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

statham-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 19, 2025

Uh oh!

Uh oh!

saturn691 commented Apr 28, 2025 •

edited

Loading

github-actions bot commented May 19, 2025 •

edited

Loading