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

[Renato Golin via Phabricator via llvm-commits]

rengolin added reviewers: compnerd, peter.smith.
rengolin 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 },
----------------
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?


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

https://reviews.llvm.org/D94557