ARM: Remove check for isAAPCS_ABI when enabling various aeabi calls

[arsenm]

Based on computeDefaultTargetABI, this appears to be redundant.

The only test using the explicit -target-abi flag with a value different
than one implied by the triple (i.e. assert(TM.isAAPCS_ABI()) under
the condition) is arm-abi-attr.ll and it doesn't stress the libcalls
used.

[arsenm]

• ARM: Remove check for isAAPCS_ABI when enabling various aeabi calls #152108 👈 (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

Based on computeDefaultTargetABI, this appears to be redundant.

The only test using the explicit -target-abi flag with a value different
than one implied by the triple (i.e. assert(TM.isAAPCS_ABI()) under
the condition) is arm-abi-attr.ll and it doesn't stress the libcalls
used.

---

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

1 Files Affected:

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

diff --git a/llvm/lib/Target/ARM/ARMISelLowering.cpp b/llvm/lib/Target/ARM/ARMISelLowering.cpp
index 7f8b4460bb814..a621e5c63eb07 100644
--- a/llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -588,8 +588,8 @@ ARMTargetLowering::ARMTargetLowering(const TargetMachine &TM_,
   }
 
   // RTLIB
-  if (TM.isAAPCS_ABI() && (TT.isTargetAEABI() || TT.isTargetGNUAEABI() ||
-                           TT.isTargetMuslAEABI() || TT.isAndroid())) {
+  if (TT.isTargetAEABI() || TT.isTargetGNUAEABI() || TT.isTargetMuslAEABI() ||
+      TT.isAndroid()) {
     // FIXME: This does not depend on the subtarget and should go directly into
     // RuntimeLibcalls. This is only here because of missing support for setting
     // the calling convention of an implementation.

[compnerd]

Hmm,wait, doesn't the GNU side set up the GNU calls?

[arsenm]

Hmm,wait, doesn't the GNU side set up the GNU calls?

I don't really see any path in here I would call the "GNU side", but this condition doesn't touch any __gnu calls. The only references to those are below under else if !TT.isOSBinFormatMachO())

[efriedma-quic]

You can directly specify the ABI using the clang "-mabi" flag on 32-bit ARM. Given that functionality exists, probably someone is using it somehow.

If we want to rip out support for using -mabi to switch the ABI on 32-bit Arm targets, I wouldn't be opposed; it's not really useful to generate code with the wrong ABI. But if we do, we should rip it out all at once, with an explicit hard error in clang, not subtly break it over time.

[rengolin]

The folks I remember were using and working in this area were @compnerd @jroelofs @kraj

I don't really see any path in here I would call the "GNU side", but this condition doesn't touch any __gnu calls. The only references to those are below under else if !TT.isOSBinFormatMachO())

There is a confusion between gnueabi and eabi because both support both calls in a way. This was a side-effect of having two, and no one really knowing which one to call, so GNU emulated EABI and ARMCC emulated GNU.

In most cases, the only difference is the function name, so both compilers had the additional symbols in some fashion. Some calls like divrem have different ABIs, so there's a wrapper on either side to move the return value (EABI returns in regs, GNU in the stack).

Technically, asking for eabi or gnueabi shouldn't make a huge difference when compiling, because both (should) provide the same functionality, but you may get linking errors. And we did. For a long time. The current state is the result of several small fixes following the gradient of least churn / crashes.

Now that the linking errors stopped (AFAIK, of course), I'm assuming whatever is the state, is "stable", but not correct. It may never be correct. Removing -mabi may be a step too far, for people using LLVM on embedded targets. @smithp35 can comment on that, probably.

[smithp35]

I've only seen the -meabi= https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-meabi used in practice. That controls whether some of the runtime calls that mirror the string.h functions are used {{__aeabi_memcpy}} or not.

The -mabi option looks to mirror a GCC option https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html#index-mabi-1 which I believe was used during the transition from the pre-ABI to post ABI (2004).

Assuming that that ATPCS, APCS and AAPCS are referring to the ABI names [1]. The APCS and ATPCS are pre ABI (2004 ish). I think ARM Linux stayed on the pre-ABI PCS for some time after that, but I don't think that would be supported anymore in GCC.

As to what these options do in clang/LLVM we'd need to reverse engineer them. I expect that keeping -mabi might aid in command-line compatibility with GCC but I wouldn't expect it to be used in embedded systems.

From the ABI https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#23terms-and-abbreviations

PCS
Procedure Call Standard.
AAPCS
Procedure Call Standard for the Arm Architecture (this standard).
APCS
Arm Procedure Call Standard (obsolete).
TPCS
Thumb Procedure Call Standard (obsolete).
ATPCS
Arm-Thumb Procedure Call Standard (precursor to this standard).

[jroelofs]

I expect that keeping -mabi might aid in command-line compatibility with GCC but I wouldn't expect it to be used in embedded systems.

haven't reviewed the rest of the PR yet, but I'm pretty sure we have firmware projects relying on this spelling of it.

[efriedma-quic]

We can "remove" support for -mabi without actually removing the flag; just only allow -mabi specifications that don't actually change the ABI.

[kraj]

Assuming that that ATPCS, APCS and AAPCS are referring to the ABI names [1]. The APCS and ATPCS are pre ABI (2004 ish). I think ARM Linux stayed on the pre-ABI PCS for some time after that, but I don't think that would be supported anymore in GCC.

I waa trying to add musl support and musleabi , speaking from Yocto's use of it, use of -mabi= has been long gone.

[davemgreen]

There are quite a few references in https://github.com/search?q=mabi%3Daapcs&type=code, I'm not sure if they do much that is useful.

[arsenm]

I don't have any particular interest in keeping or removing -mabi; my issue is this is just untested complexity in the backend. If there was test coverage there wouldn't be an issue