[ARM][Compiler-rt] Fix AEABI builtins to correctly pass arguments to non-AEABI functions on HF targets

[iid_iunknown]

Summary

This is a patch for PR34167.

On HF targets functions like __{eq,lt,le,ge,gt}df2 and __{eq,lt,le,ge,gt}sf2 expect their arguments to be passed in d/s registers, while some of the AEABI builtins pass them in r registers.

[peter.smith]

The code changes look fine to me. I haven't managed to find an existing test case that explicitly calls aeabi_fcmp and aeabi_dcmp for a hard float target. Would it be possible to add one so that we can make sure that we catch any future problems?

lib/builtins/arm/aeabi_dcmp.S

22

Is it worth making the macro name less generic as it only applies to this specific case, and could clash with a future definition at a higher level scope?

[compnerd]

Um, when LLVM makes the invocation, it will assume that s0/s1 are not clobbered, even though it is a function call. Shouldn't we be explicitly saving and restoring s0/s1 in the ARM HF cases?

[peter.smith]

By my reading of the AAPCS http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf (5.1.2.1 VFP register usage conventions (VFP v2, v3 and the Advanced SIMD Extension)
"Registers s16-s31 (d8-d15, q4-q7) must be preserved across subroutine calls; registers s0-s15 (d0-d7, q0-q3) do not need to be preserved (and can be used for passing arguments or returning results in standard procedure-call variants). Registers d16-d31 (q8-q15), if present, do not need to be preserved."
So I don't think that when calling __aeabi_fcmp the caller can rely on s0 and s1 not being clobbered. There are some other examples in compiler-rt that I've seen that do this for example comparef2.S.

I can try and double check this tomorrow.

[asl]

In D36675#840918, @compnerd wrote:

Um, when LLVM makes the invocation, it will assume that s0/s1 are not clobbered, even though it is a function call. Shouldn't we be explicitly saving and restoring s0/s1 in the ARM HF cases?

Not quite. Here is the full list of callee-saved registers:

def CSR_AAPCS : CalleeSavedRegs<(add LR, R11, R10, R9, R8, R7, R6, R5, R4,
                                     (sequence "D%u", 15, 8))>;

Everything else is callee clobbered and therefore should be saved by a caller.

[iid_iunknown]

Thanks for the feedback, Peter!

Is it worth making the macro name less generic

Good point. I will update the patch.

I haven't managed to find an existing test case that explicitly calls aeabi_fcmp and aeabi_dcmp for a hard float target.

This problem was caught using the standard compiler-rt test suite :) The tests are test/builtins/Unit/arm/aeabi_cfcmpeq_test.c and test/builtins/Unit/arm/aeabi_cdcmpeq_test.c.
For aeabi_cfcmpeq_test.c the call sequence looks like this:

test -> __aeabi_cfcmpeq -> __aeabi_cfcmple -> __aeabi_fcmpeq -> __eqsf2

However, I can't find an LLVM buildbot that builds and runs the compiler-rt tests for ARM.

[iid_iunknown]

Address review remarks: make the macro names less generic.

[peter.smith]

Thanks for making the changes. If this is covered by an existing test case I'm happy.

This problem was caught using the standard compiler-rt test suite :) The tests are test/builtins/Unit/arm/aeabi_cfcmpeq_test.c and test/builtins/Unit/arm/aeabi_cdcmpeq_test.c.
For aeabi_cfcmpeq_test.c the call sequence looks like this:

test -> __aeabi_cfcmpeq -> __aeabi_cfcmple -> __aeabi_fcmpeq -> __eqsf2

However, I can't find an LLVM buildbot that builds and runs the compiler-rt tests for ARM.

Interesting; I've used a cross-compilation of compiler-rt for ARM using qemu-arm to run the tests, sadly when I look at the logs in more detail I get

UNSUPPORTED: Builtins-armhf-linux :: arm/aeabi_cdcmple_test.c (3 of 194)
Test requires the following unavailable features: arm-target-arch || armv6m-target-arch

I'll need to work out why my target (default armv7--linux-gnueabihf) got rejected. As an aside it would be interesting to know what target you used?

Ideally it would be great to get compiler-rt able to build and run its tests for all the ARM architectures running via QEMU as a buildbot that could build all the supported platforms would be much faster than a native build-bot, and may be the only way for the M class builds.

[iid_iunknown]

I'll need to work out why my target (default armv7--linux-gnueabihf) got rejected. As an aside it would be interesting to know what target you used?

Yes, I had to manually add arm-target-arch to available features to enable these tests.
Seems we can fix this by handling the ARM architectures similarly to x86 (test/lit.common.cfg):

if target_arch in ['x86_64', 'i386', 'i686']:
  config.available_features.add('x86-target-arch')

For ARM we now have only

config.available_features.add(target_arch + '-target-arch')

so the armv7--linux-gnueabihf triple gives us armv7-target-arch that does not match the test's requirements.

[peter.smith]

In D36675#842814, @iid_iunknown wrote:

I'll need to work out why my target (default armv7--linux-gnueabihf) got rejected. As an aside it would be interesting to know what target you used?

Yes, I had to manually add arm-target-arch to available features to enable these tests.
Seems we can fix this by handling the ARM architectures similarly to x86 (test/lit.common.cfg):

if target_arch in ['x86_64', 'i386', 'i686']:
  config.available_features.add('x86-target-arch')

For ARM we now have only

config.available_features.add(target_arch + '-target-arch')

so the armv7--linux-gnueabihf triple gives us armv7-target-arch that does not match the test's requirements.

Thank you very much for the tip. Yes that approach looks like it would be worthwhile checking out.

[iid_iunknown]

Hi Peter,

Would you mind if I commit this patch?
Or you would prefer the lit changes that enable the AEABI tests for armv7 and some other ARM targets to be included into this patch?
Thanks.

[peter.smith]

I'm ok with this, I've set the status to approved, although it will be worth waiting a day or so to see if compnerd has any objections.

[asl]

Given that this is regression from 3.9 and it's release-critical, I'd would suggest to commit it now. After all, the patch was in review for more than a week.

[iid_iunknown]

Peter, Anton,
Thank you very much for helping with this!

I am going to commit the patch shortly.

[compnerd]

LG, thanks for fixing this.

lib/builtins/arm/aeabi_dcmp.S

 //   int result = __{eq,lt,le,ge,gt}df2(a, b);
 //   if (result {==,<,<=,>=,>} 0) {
 //     return 1;
 //   } else {
 //     return 0;
 //   }
 // }
 
+#if defined(COMPILER_RT_ARMHF_TARGET)
+#  define CONVERT_DCMP_ARGS_TO_DF2_ARGS                    \
+        vmov      d0, r0, r1                     SEPARATOR \
+        vmov      d1, r2, r3
+#else
+#  define CONVERT_DCMP_ARGS_TO_DF2_ARGS
+#endif
+
 #define DEFINE_AEABI_DCMP(cond)                            \
         .syntax unified                          SEPARATOR \
         .p2align 2                               SEPARATOR \
 DEFINE_COMPILERRT_FUNCTION(__aeabi_dcmp ## cond)           \
         push      { r4, lr }                     SEPARATOR \
+        CONVERT_DCMP_ARGS_TO_DF2_ARGS            SEPARATOR \
         bl        SYMBOL_NAME(__ ## cond ## df2) SEPARATOR \
         cmp       r0, #0                         SEPARATOR \
         b ## cond 1f                             SEPARATOR \
         movs      r0, #0                         SEPARATOR \
         pop       { r4, pc }                     SEPARATOR \
 1:                                               SEPARATOR \
         movs      r0, #1                         SEPARATOR \
         pop       { r4, pc }                     SEPARATOR \

lib/builtins/arm/aeabi_fcmp.S

 //   int result = __{eq,lt,le,ge,gt}sf2(a, b);
 //   if (result {==,<,<=,>=,>} 0) {
 //     return 1;
 //   } else {
 //     return 0;
 //   }
 // }
 
+#if defined(COMPILER_RT_ARMHF_TARGET)
+#  define CONVERT_FCMP_ARGS_TO_SF2_ARGS                    \
+        vmov      s0, r0                         SEPARATOR \
+        vmov      s1, r1
+#else
+#  define CONVERT_FCMP_ARGS_TO_SF2_ARGS
+#endif
+
 #define DEFINE_AEABI_FCMP(cond)                            \
         .syntax unified                          SEPARATOR \
         .p2align 2                               SEPARATOR \
 DEFINE_COMPILERRT_FUNCTION(__aeabi_fcmp ## cond)           \
         push      { r4, lr }                     SEPARATOR \
+        CONVERT_FCMP_ARGS_TO_SF2_ARGS            SEPARATOR \
         bl        SYMBOL_NAME(__ ## cond ## sf2) SEPARATOR \
         cmp       r0, #0                         SEPARATOR \
         b ## cond 1f                             SEPARATOR \
         movs      r0, #0                         SEPARATOR \
         pop       { r4, pc }                     SEPARATOR \
 1:                                               SEPARATOR \
         movs      r0, #1                         SEPARATOR \
         pop       { r4, pc }                     SEPARATOR \