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

[Kevin Peizner via Phabricator via llvm-commits]

kevinpeizner added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:601
       { 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 },
----------------
rengolin wrote:
> kevinpeizner wrote:
> > rengolin wrote:
> > > 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.
> > 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.
> 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94557/new/

https://reviews.llvm.org/D94557