[ARM] Fixed incorrect lowering when using GNUEABI (libgcc) and 16bit floats

[kevinpeizner]

Summary

When compiling code that uses 16bit floating point and the compilation
targets GNUEABI, the intrinsic __gnu_d2h_ieee from libgcc should be
used rather than __aeabi_d2h which is not available in libgcc.

An example of this issue can be found in this Compiler Explorer
instance (using clang trunk as of Jan 04, 2020):
https://godbolt.org/z/anj1Y6

In the Compiler Explorer example, if you switch the target between
armv7-unknown-linux-gnueabihf and armv7-unknown-linux-eabihf you can
see many intrinsics are correctly swapped except for __aeabi_d2h.

This commit makes correct the code comment (line 710 in
ARMISelLowering.cpp as of commit 32c47ebef18):

  // In EABI, these functions have an __aeabi_ prefix, but in GNUEABI they have
  // a __gnu_ prefix (which is the default).

[rengolin]

Also, please use diff -U9999 as documented: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

llvm/lib/Target/ARM/ARMISelLowering.cpp

601

This doesn't look like the right fix. You're just replacing for all, no? Your changes to the tests don't show it can still emit EABI intrinsics with the right ABI flag.

[kevinpeizner]

Updated patch with diff -U9999

[kevinpeizner]

llvm/lib/Target/ARM/ARMISelLowering.cpp

601

The i16 @test_to_fp16(double %in) in llvm/test/CodeGen/ARM/fp16.ll test checks for different intrinsics based on the EABI flag. For example, when the test runs with
; RUN: llc -mtriple=armv7a--none-eabi < %s | FileCheck --check-prefix=CHECK --check-prefix=CHECK-HARDFLOAT-EABI %s then the test will verify that bl __aeabi_d2h is emitted rather than bl __gnu_d2h_ieee.

I interpret this to mean that my changes still emit EABI intrinsics with the right ABI flag. This is my first contribution to the LLVM Project so please let me know if this is incorrect.

The intent is to emit __gnu_d2h_ieee by default while allowing __aeabi_d2h to be emitted when EABI is targeted.

720

Here __gnu_d2h_ieee is replaced with __aeabi_d2h if and only if EABI is targeted.

[rengolin]

llvm/lib/Target/ARM/ARMISelLowering.cpp

601

You're right, I didn't catch the other tests due to the lack of context. But this still looks like the wrong place. It's been a long time I touched this part of the code, but it being the only "gnu" replacement where all the rest is still "eabi" looks wrong.

There must be another place where the "gnu" calls are being replaced and probably just adding "d2h" there would do the trick?

[kevinpeizner]

llvm/lib/Target/ARM/ARMISelLowering.cpp

601

Grepping for isTargetGNUAEABI shows that there is no conditional block for GNUAEABI && !AEABI. On the other hand, grepping for isTargetAEABI shows that there is one conditional block for AEABI && !GNUAEABI (see line 713).

I can add a new conditional block to achieve the same result, if that is preferred. However, based on the comment at line 712, I thought changing the default intrinsic to __gnu_d2h_ieee made the most sense and minimized code change.

[compnerd]

I agree with Renato, this seems incorrect. In the EABI mode, we should prefer the EABI symbols.

[kevinpeizner]

In D94557#2498984, @compnerd wrote:

I agree with Renato, this seems incorrect. In the EABI mode, we should prefer the EABI symbols.

Maybe I'm miss understanding, but this change ensures EABI symbols are still used when EABI is targeted. The fp16.ll regression test verifies this is the case, no?

[kevinpeizner]

Would it be better to default to EABI symbols and conditionally use GNU symbols based on the target? I can certain switch to that approach, but I didn't go that way initially since this code block implies GNU symbols should be the default:

// In EABI, these functions have an __aeabi_ prefix, but in GNUEABI they have
// a __gnu_ prefix (which is the default).
if (Subtarget->isTargetAEABI()) {
  static const struct {
    const RTLIB::Libcall Op;
    const char * const Name;
    const CallingConv::ID CC;
  } LibraryCalls[] = {
    { RTLIB::FPROUND_F32_F16, "__aeabi_f2h", CallingConv::ARM_AAPCS },
    { RTLIB::FPROUND_F64_F16, "__aeabi_d2h", CallingConv::ARM_AAPCS },
    { RTLIB::FPEXT_F16_F32, "__aeabi_h2f", CallingConv::ARM_AAPCS },
  };

[fdelagarza]

Here's some observations that might help figure out what path to take

According to the discussion on this ticket , there are only four arm __aeabi prefixed functions in compiter-rt that aren't present in libgcc:

__aeabi_memcpy
__aeabi_memmove
__aeabi_memset
__aeabi_d2h

memcpy, memmove and memset default to being lowered to the non __aebi prefixed functions in include/llvm/IR/RuntimeLibcalls.def:

HANDLE_LIBCALL(MEMCPY, "memcpy")
HANDLE_LIBCALL(MEMMOVE, "memmove")
HANDLE_LIBCALL(MEMSET, "memset")

A code path in ARMISelLowering.cpp switches them to __aeabi_memcpy, __aeabi_memmove and __aeabi_memset if the "EABIVersion" option is set to EABI4 or EABI5.

Also in include/llvm/IR/RuntimeLibcalls.def, the FPROUND_F32_F16 and FPROUND_F32_F16 library calls default to __gnu prefixed functions:

HANDLE_LIBCALL(FPEXT_F16_F32, "__gnu_h2f_ieee")
HANDLE_LIBCALL(FPROUND_F32_F16, "__gnu_f2h_ieee")
HANDLE_LIBCALL(FPROUND_F64_F16, "__truncdfhf2")

Code in ARMISelLowering.cpp is currently unconditionally switching the FPROUND_F64_F16 default lowering to __aeabi_d2h. Kevin's change would make it default to __gnu_d2h_ieee

There is a code path in ARMISelLowering.cpp that changes FPROUND_F32_F16, FPROUND_F64_F16, and FPEXT_F16_F32 to the __eabui prefixed functions when EABI4 or EABI5 is requested (mentioned in Kevin's last comment)

To me it seems the proposed change would make the default FPROUND_F64_F16 lowering more consistent with that of FPEXT_F16_F32 and FPROUND_F32_F16

[fdelagarza]

That said, I don't know if making FPROUND_F64_F16 consistent with FPEXT_F16_F32 and FPROUND_F32_F16 is necessarily the right thing to do

Being explicit with a Subtarget->isTargetGNUAEABI() code branch also seems like a good option and would minimize change to other subtargets

For example, I'm not sure what the android triplet is, but the swift stdlib is already adding an implementation for __aeabi_d2h for android armv7 platforms: https://github.com/apple/swift/blob/cd2d4cc287814c39f33ee1f57ec05aac3f0537e4/stdlib/public/runtime/Float16Support.cpp#L168

[psmith]

As I understand it, this is about what library call should be made when there is an eabi target such as arm-none-eabi which should target the Run Time ABI for the Arm Architecture https://github.com/ARM-software/abi-aa/blob/master/rtabi32/rtabi32.rst and other Arm targets such as Linux or Android. I think if we're arguing from the specification then clang is compliant with the eabi, so relocatable objects produced by clang should be compatible with eabi compliant toolchains that supply the eabi functions, this includes non GNU toolchains as well like IAR.

I believe that in this case GNUEABI is intending to be compliant with eabi, as it does supply aeabi prefixed functions. I think in this case it is likely an omission in gcc/libgcc as even arm-none-eabi-gcc with -mfp16-format=ieee which is supposed to be targeting the bare-metal eabi uses __gnu_d2h_ieee however even if this is fixed then there will be lots of libgcc libraries without the function.

I've asked the Arm GNU team if they can remember why the aeabi functions were not used for FP16 in GCC https://gcc.gnu.org/pipermail/gcc-patches/2016-November/462046.html

Having said that, even if this is working around a bug in libgcc, if this is restricted to GNUEABI then it may be the right thing to do as there will be many existing libgcc libraries without it.

As an aside, the __aeabi_memcpy, __aeabi_memmove, __aeabi_memset functions are a bit of a special case in that these are intended to be "optimized" functions, all libraries have memcpy, memmove and memset so it is always safe to put out a call to one of these and be compatible with everything. Emitting a call to __gnu_d2h_ieee is unlikely to be present in a toolchain not trying to be compatible with GCC so it is more of a risk to do so.

llvm/lib/Target/ARM/ARMISelLowering.cpp

       // Conversions between floating types.
       // RTABI chapter 4.1.2, Table 7
       { RTLIB::FPROUND_F64_F32, "__aeabi_d2f", CallingConv::ARM_AAPCS, ISD::SETCC_INVALID },
-      { RTLIB::FPROUND_F64_F16, "__aeabi_d2h", CallingConv::ARM_AAPCS, ISD::SETCC_INVALID },
+      { RTLIB::FPROUND_F64_F16, "__gnu_d2h_ieee", CallingConv::ARM_AAPCS, ISD::SETCC_INVALID },
       { RTLIB::FPEXT_F32_F64,   "__aeabi_f2d", CallingConv::ARM_AAPCS, ISD::SETCC_INVALID },
 
       // Integer to floating-point conversions.

llvm/test/CodeGen/ARM/fp16.ll

   %val = call i16 @llvm.convert.to.fp16.f64(double %in)
 ; CHECK-HARDFLOAT-EABI: bl __aeabi_d2h
 
-; CHECK-HARDFLOAT-GNU: bl __aeabi_d2h
+; CHECK-HARDFLOAT-GNU: bl __gnu_d2h_ieee
 
 ; CHECK-FP16-SAFE: bl __aeabi_d2h
 

 
 ; CHECK-SOFTFLOAT-EABI: bl __aeabi_d2h
 
-; CHECK-SOFTFLOAT-GNU: bl __aeabi_d2h
+; CHECK-SOFTFLOAT-GNU: bl __gnu_d2h_ieee
   ret i16 %val
 }