ARM: Remove redundant or buggy config of __aeabi_d2h

[arsenm]

This was set if TT.isTargetAEABI(). This was previously set above
if TM.isAAPCS_ABI() && (TT.isTargetAEABI() || TT.isTargetGNUAEABI() || TT.isTargetMuslAEABI() || TT.isAndroid()).

So this could differ based on a manually specified -target-abi flag due
to the isAAPCS_ABI part of the original condition. I'm guessing
these should be consistent, so either this second group of setLibcallImpl
calls should have been guarded by the isAAPCS_ABI check, or the first
condition should remove it.

There doesn't appear to be any meaningful test coverage using the
manually specified ABI option, so #152108 tries to remove it

[arsenm]

• ARM: Remove redundant or buggy config of __aeabi_d2h #152126 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-backend-arm

Author: Matt Arsenault (arsenm)

Changes

This was set if TT.isTargetAEABI(). This was previously set above
if TM.isAAPCS_ABI() && (TT.isTargetAEABI() || TT.isTargetGNUAEABI() || TT.isTargetMuslAEABI() || TT.isAndroid()).

So this could differ based on a manually specified -target-abi flag due
to the isAAPCS_ABI part of the original condition. I'm guessing
these should be consistent, so either this second group of setLibcallImpl
calls should have been guarded by the isAAPCS_ABI check, or the first
condition should remove it.

There doesn't appear to be any meaningful test coverage using the
manually specified ABI option, so #152108 tries to remove it

---

Full diff: https://github.com/llvm/llvm-project/pull/152126.diff

1 Files Affected:

• (modified) llvm/lib/Target/ARM/ARMISelLowering.cpp (-1)

diff --git a/llvm/lib/Target/ARM/ARMISelLowering.cpp b/llvm/lib/Target/ARM/ARMISelLowering.cpp
index 7f8b4460bb814..18766c8c6befa 100644
--- a/llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -737,7 +737,6 @@ ARMTargetLowering::ARMTargetLowering(const TargetMachine &TM_,
       const RTLIB::LibcallImpl Impl;
     } LibraryCalls[] = {
         {RTLIB::FPROUND_F32_F16, RTLIB::__aeabi_f2h},
-        {RTLIB::FPROUND_F64_F16, RTLIB::__aeabi_d2h},
         {RTLIB::FPEXT_F16_F32, RTLIB::__aeabi_h2f},
     };
 

[compnerd]

I think that the other case should be changed. The GNU EABI should use the GNU variant. The AEABI implementation might be compatible with APCS, but definitely is compatible with AAPCS.

[arsenm]

I think that the other case should be changed. The GNU EABI should use the GNU variant. The AEABI implementation might be compatible with APCS, but definitely is compatible with AAPCS.

I'm not sure exactly what you're saying; I'm not trying to change the used call or behavior, only try to make the exact conditions where the calls are currently used more obvious

[rengolin]

This looks like a leftover of some other partial change a long time ago. I've touched that area a long time ago, when gnueabi and eabi didn't have super clear distinction between each other.

How can we test this?

[efriedma-quic]

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120897 for a summary of the history here; it's very messy. Really, we should be calling __gnu_d2h_ieee on targets using libgcc because __aeabi_d2h doesn't exist, but the patch (https://reviews.llvm.org/D94557) never got merged.

I think we have sufficient test coverage in llvm/test/CodeGen/ARM/fp16.ll, to detect if anyone accidentally makes further changes here.

This seems fine for now; if someone wants to revive D94557, they can do it once all the TableGen changes are in.

LGTM