This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Use correct calling convention for libm.
AbandonedPublic

Authored by efriedma on Sep 19 2017, 12:28 PM.

Details

Summary

-mfloat-abi doesn't control the calling convention for compiler-rt: it's built with the compiler, so it can only have one calling convention for a given target. -mfloat-abi does control the calling convention for any function provided by libm: libm is built by the user, so the user should have control over its calling convention. See https://bugs.llvm.org/show_bug.cgi?id=30543 for previous discussion on the subject.

I don't really like the big list, but I'm not sure there's a better way to handle this.

Fixes https://bugs.llvm.org//show_bug.cgi?id=34530.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Sep 19 2017, 12:28 PM
compnerd added inline comments.Sep 19 2017, 1:12 PM
lib/Target/ARM/ARMISelLowering.cpp
313

I think that it would be better to have a table and just walk that rather than doing this explicitly.

peter.smith edited edge metadata.Sep 20 2017, 4:10 AM

My apologies in advance if I've missed something. If I understand this correctly we are tying the calling convention used for compiler-rt to the triple, but for user code, and with this change for builtins provided by libm) we allow -mfloat-abi to determine the calling convention? I think that this can be simplified, but it does require removing the association between triple and compiler-rt float-abi, which I don't think should be there. I think that the behaviour for targets intending to follow the ABI for the Arm Architecture (i.e. not Apple or Microsoft) should be:

  • The aeabi functions described in RTABI chapter 4.1.2 required to use AAPCS should always use ARM_AAPCS.
  • A triple with a suffix of HF such as arm-linux-gnueabihf implies ARM_AAPCS_VFP for all other builtins, otherwise ARM_AAPCS is used.
  • The -mfloat-abi=hard should override whatever is in the triple for all other builtins. There should be no special treatment for compiler-rt.

We would then not need to maintain a list of builtins with floating point parameters defined outside compiler-rt.

My argument for not giving special treatment to compiler-rt is:

  • The HF in the gcc target is a naming convention followed by some but not all distributions, my understanding is that Debian uses it but Red Hat does not, or at least did not.
  • The -mfloat-abi overrides the default float-abi for all libraries including libgcc.
  • If there is a good, or just legacy, reason to give compiler-rt special treatment, perhaps it should be controllable by an option.

For example I have an arm-linux-gnueabi and arm-linux-gnueabihf linaro toolchains installed. With the source program:

#include <math.h>
/* defined in compiler-rt */
double _Complex ComplexMul(double _Complex c, double _Complex d) {
  return c * d;
}

/* defined in libm */
float SquareRoot(float f) {
  return sqrt(f);
}

With a soft-float gcc, with an extra -I to find the missing gnu/stubs-hard.h that aren't included in the soft-float distribution:

arm-linux-gnueabi-gcc -c t.c -mfloat-abi=hard -I /work/arm-linux-gnueabihf/libc/usr/include

I get both functions called with ARM-AAPCS-VFP

p.s. I note that div[d,s,t]f3.c in compiler-rt calls out to sqrt, would be an interesting test case to make sure that we get right.

I was working under the assumption the behavior test/CodeGen/Thumb2/intrinsics-cc.ll is testing for is is correct; if it's wrong, I'm happy to change the patch. (I probably should have tested gcc before posting this; looks like clang's current handling of __builtin_powi is different from gcc.)

compnerd edited edge metadata.Sep 20 2017, 3:12 PM

@peter.smith the reason that the complexity exists currently is interoperability with gcc. If the triple is not hf, libgcc does not use AAPCS_VFP_CC. This is the source of the problem. Your mapping is correct though:

  • RTABI §4.1.2 functions must use AAPCS_VFP
  • builtins (excluding RTABI §4.1.2 symbols) use AAPCS_VFP_CC if the target environment is hf, AAPCS_CC otherwise
  • all remaining functions are determined based on -mfloat-abi=.

I believe the triple convention differences impact the CC for libgcc on the various targets as well. It just is that it is usually statically linked so people don't realize the difference.

rengolin edited edge metadata.Sep 20 2017, 3:37 PM

Do you know what would be cool? A gcc-compatibility checker test... Something that would have the most problematic cases in C and asm files and would be compiled with multiple versions of gcc and clang triples, then compared.

It's probably a very boring job to do, but once created, we wouldn't have to rehash those silly legacy ABI craziness... :(

Who knows, maybe we can even show other people how silly those things are, and they can even fix on their compiler! (nah, just kidding...)

@peter.smith the reason that the complexity exists currently is interoperability with gcc. If the triple is not hf, libgcc does not use AAPCS_VFP_CC. This is the source of the problem. Your mapping is correct though:

  • RTABI §4.1.2 functions must use AAPCS_VFP
  • builtins (excluding RTABI §4.1.2 symbols) use AAPCS_VFP_CC if the target environment is hf, AAPCS_CC otherwise
  • all remaining functions are determined based on -mfloat-abi=.

I believe the triple convention differences impact the CC for libgcc on the various targets as well. It just is that it is usually statically linked so people don't realize the difference.

A further complexity is that I lose track of the product of {clang, gcc} * { compiler-rt, libgcc} ...

From what I understand of gcc, the calling convention used (outside of RTABI 4.1.2) is that -mfloat-abi will override the default given in the builtin specs file, there isn't anything special about libgcc. The -mfloat-abi won't change the include and default library paths given by the builtin specs file so if I use -mfloat-abi=hard from an arm-linux-gnueabi-gcc it will select the soft-float libgcc.a and other libraries. With ld.bfd this gives me a link error due to the BuildAttribute clash between the static libraries and my object.

arm-linux-gnueabi/bin/ld: error: /tmp/ccPq8Sq6.o uses VFP register arguments, t.axf does not

So I think clang/llvm is doing something different than gcc here, albeit in gcc if the user does use a different -mfloat-abi from the sysroot they will get an error from the static libraries, interestingly shared-libraries don't have BuildAttributes so there is no checking a shared-library mismatch.

Taking the discussion back to the patch, I think that it is the right thing to do to call builtins in libm with -mfloat-abi, so I've no objections to the patch. I'm not sure that making a special case out of the builtins in compiler-rt/libgcc is the right thing to do but this is probably best handled in a different review; there is a huge amount of emulation logic in clang based on target environment that I don't understand, so I could easily be missing something.

See https://reviews.llvm.org/D38299; from the discussion here and my testing, it seems like that patch more closely matches what gcc actually does.

efriedma abandoned this revision.Oct 23 2017, 3:10 PM

Abandoning in favor of D38299