[builtins] Fix ABI-incompatibility with GCC for floating-point compare

[arichardson]

Summary

While implementing support for the float128 routines on x86_64, I noticed
that __builtin_isinf() was returning true for 128-bit floating point
values that are not infinite when compiling with GCC and using the
compiler-rt implementation of the soft-float comparison functions.
After stepping through the assembly, I discovered that this was caused by
GCC assuming a sign-extended 64-bit -1 result, but our implementation
returns an enum (which then has zeroes in the upper bits) and therefore
causes the comparison with -1 to fail.

Fix this by using a CMP_RESULT typedef and add a static_assert that it
matches the GCC soft-float comparison return type when compiling with GCC
(GCC has a libgcc_cmp_return mode that can be used for this purpose).

Also move the 3 copies of the same code to a shared .inc file.

[arichardson]

compiler-rt/lib/builtins/fp_compare_impl.inc

26–32

I'm not sure there's much point keeping these enums named, I could just change the return type of the helper functions to CMP_RESULT?

[arichardson]

• Remove name from LE_RESULT/GE_RESULT enum and return CMP_RESULT directly

[compnerd]

compiler-rt/lib/builtins/fp_compare_impl.inc

15

I think that we should use intptr_t instead. That ensures that this remains the same size on LLP64 environments.

19

Can we use the reserved spelling for mode please? (__mode__)

26–32

The named result constants is the useful thing IMO. Changing the return types to CMP_RESULT is fine. However, please put a trailing comma on the enum cases so that it is similar to the GE case.

[arichardson]

compiler-rt/lib/builtins/fp_compare_impl.inc

15

Good point, I did not think about LLP64 platforms, I'm not sure what GCC would use there?

Unfortunately that is incorrect for the environment I mostly work with (CHERI). In our environment pointers/intptr_t are 128+1-bit capabilities but long/long long is still 64 bits (we also have a 32-bit version where pointers are 64+1 bits).

intptr_t would happen to work on CHERI-RISC-V and Arm Morello due to a merged register file where it will return a value with the other 65 bits zeroed. However, for our MIPS adaptation we have a split register file where intptr_t and long are passed in different registers. In this case using intptr_t would place the return value in register $c3 instead of the expected $v0.

If long is incorrect for LLP64, I think ptrdiff_t (or the non-standard ssize_t) might work on all supported architectures (we don't do segmented AS) despite not technically being correct.

19

Sure, will update.

[arichardson]

• handle LLP64 ABIs
• shared implementation of __unord{s,d,t,}f2
• clang-format
• add missing comma
• use mode

[arichardson]

compiler-rt/lib/builtins/fp_compare_impl.inc

15

I just tested with x86_64-w64-mingw32-gcc-10.2.0 but there doesn't seem to be a builtin macro to check for LLP64 (other than _WIN64), so I went for __SIZEOF_POINTER__ == 8 && __SIZEOF_LONG__ == 4

[jrtc27]

compiler-rt/lib/builtins/fp_compare_impl.inc

15

intptr_t is definitely wrong, it's always an integer and never a pointer (see CHERI which cares about the distinction; using intptr_t would give us inefficient code gen). In reality ssize_t is about as good as you can get, and would suffice other than things like the AArch64 weirdness below.

19

AArch64 looks like it's using SImode i.e. a 32-bit int (aarch64_libgcc_cmp_return_mode). In practice I assume the calling convention will mean that extending too much doesn't matter (although perhaps may cause weirdness if GCC assumes the upper half of the value is 0 not sign-extended and relies on that for later computation?), but the static assertion will fail here.

[arichardson]

• Fix CMP_RESULT typedef on aarch64

[arichardson]

compiler-rt/lib/builtins/fp_compare_impl.inc

20

We could probably avoid the LLP64 ifdef by using typedef int CMP_RESULT __attribute__((__mode__(__word__))); but I'm not particularly keen on using this rather ugly GCC attribute for non-GCC builds.

[arichardson]

ping. Is the current version okay or should I use something like typedef int CMP_RESULT __attribute__((__mode__(word))); instead?

[arichardson]

If there are no further comments, I will commit this early next week.

[aykevl]

This change resulted in a regression for AVR: D124939

compiler-rt/lib/builtins/comparedf2.c

 // Note that __ledf2( ) and __gedf2( ) are identical except in their handling of
 // NaN values.
 //
 //===----------------------------------------------------------------------===//
 
 #define DOUBLE_PRECISION
 #include "fp_lib.h"
 
-enum LE_RESULT { LE_LESS = -1, LE_EQUAL = 0, LE_GREATER = 1, LE_UNORDERED = 1 };
+#include "fp_compare_impl.inc"
 
-COMPILER_RT_ABI enum LE_RESULT __ledf2(fp_t a, fp_t b) {
-
-  const srep_t aInt = toRep(a);
-  const srep_t bInt = toRep(b);
-  const rep_t aAbs = aInt & absMask;
-  const rep_t bAbs = bInt & absMask;
-
-  // If either a or b is NaN, they are unordered.
-  if (aAbs > infRep || bAbs > infRep)
-    return LE_UNORDERED;
-
-  // If a and b are both zeros, they are equal.
-  if ((aAbs | bAbs) == 0)
-    return LE_EQUAL;
-
-  // If at least one of a and b is positive, we get the same result comparing
-  // a and b as signed integers as we would with a floating-point compare.
-  if ((aInt & bInt) >= 0) {
-    if (aInt < bInt)
-      return LE_LESS;
-    else if (aInt == bInt)
-      return LE_EQUAL;
-    else
-      return LE_GREATER;
-  }
-
-  // Otherwise, both are negative, so we need to flip the sense of the
-  // comparison to get the correct result.  (This assumes a twos- or ones-
-  // complement integer representation; if integers are represented in a
-  // sign-magnitude representation, then this flip is incorrect).
-  else {
-    if (aInt > bInt)
-      return LE_LESS;
-    else if (aInt == bInt)
-      return LE_EQUAL;
-    else
-      return LE_GREATER;
-  }
-}
+COMPILER_RT_ABI CMP_RESULT __ledf2(fp_t a, fp_t b) { return __leXf2__(a, b); }
 
 #if defined(__ELF__)
 // Alias for libgcc compatibility
 COMPILER_RT_ALIAS(__ledf2, __cmpdf2)
 #endif
 COMPILER_RT_ALIAS(__ledf2, __eqdf2)
 COMPILER_RT_ALIAS(__ledf2, __ltdf2)
 COMPILER_RT_ALIAS(__ledf2, __nedf2)
 
-enum GE_RESULT {
-  GE_LESS = -1,
-  GE_EQUAL = 0,
-  GE_GREATER = 1,
-  GE_UNORDERED = -1 // Note: different from LE_UNORDERED
-};
-
-COMPILER_RT_ABI enum GE_RESULT __gedf2(fp_t a, fp_t b) {
-
-  const srep_t aInt = toRep(a);
-  const srep_t bInt = toRep(b);
-  const rep_t aAbs = aInt & absMask;
-  const rep_t bAbs = bInt & absMask;
-
-  if (aAbs > infRep || bAbs > infRep)
-    return GE_UNORDERED;
-  if ((aAbs | bAbs) == 0)
-    return GE_EQUAL;
-  if ((aInt & bInt) >= 0) {
-    if (aInt < bInt)
-      return GE_LESS;
-    else if (aInt == bInt)
-      return GE_EQUAL;
-    else
-      return GE_GREATER;
-  } else {
-    if (aInt > bInt)
-      return GE_LESS;
-    else if (aInt == bInt)
-      return GE_EQUAL;
-    else
-      return GE_GREATER;
-  }
-}
+COMPILER_RT_ABI CMP_RESULT __gedf2(fp_t a, fp_t b) { return __geXf2__(a, b); }
 
 COMPILER_RT_ALIAS(__gedf2, __gtdf2)
 
-COMPILER_RT_ABI int
-__unorddf2(fp_t a, fp_t b) {
-    const rep_t aAbs = toRep(a) & absMask;
-    const rep_t bAbs = toRep(b) & absMask;
-    return aAbs > infRep || bAbs > infRep;
+COMPILER_RT_ABI CMP_RESULT __unorddf2(fp_t a, fp_t b) {
+  return __unordXf2__(a, b);
 }
 
 #if defined(__ARM_EABI__)
 #if defined(COMPILER_RT_ARMHF_TARGET)
 AEABI_RTABI int __aeabi_dcmpun(fp_t a, fp_t b) { return __unorddf2(a, b); }
 #else
 COMPILER_RT_ALIAS(__unorddf2, __aeabi_dcmpun)
 #endif

compiler-rt/lib/builtins/comparesf2.c

 // Note that __lesf2( ) and __gesf2( ) are identical except in their handling of
 // NaN values.
 //
 //===----------------------------------------------------------------------===//
 
 #define SINGLE_PRECISION
 #include "fp_lib.h"
 
-enum LE_RESULT { LE_LESS = -1, LE_EQUAL = 0, LE_GREATER = 1, LE_UNORDERED = 1 };
+#include "fp_compare_impl.inc"
 
-COMPILER_RT_ABI enum LE_RESULT __lesf2(fp_t a, fp_t b) {
-
-  const srep_t aInt = toRep(a);
-  const srep_t bInt = toRep(b);
-  const rep_t aAbs = aInt & absMask;
-  const rep_t bAbs = bInt & absMask;
-
-  // If either a or b is NaN, they are unordered.
-  if (aAbs > infRep || bAbs > infRep)
-    return LE_UNORDERED;
-
-  // If a and b are both zeros, they are equal.
-  if ((aAbs | bAbs) == 0)
-    return LE_EQUAL;
-
-  // If at least one of a and b is positive, we get the same result comparing
-  // a and b as signed integers as we would with a fp_ting-point compare.
-  if ((aInt & bInt) >= 0) {
-    if (aInt < bInt)
-      return LE_LESS;
-    else if (aInt == bInt)
-      return LE_EQUAL;
-    else
-      return LE_GREATER;
-  }
-
-  // Otherwise, both are negative, so we need to flip the sense of the
-  // comparison to get the correct result.  (This assumes a twos- or ones-
-  // complement integer representation; if integers are represented in a
-  // sign-magnitude representation, then this flip is incorrect).
-  else {
-    if (aInt > bInt)
-      return LE_LESS;
-    else if (aInt == bInt)
-      return LE_EQUAL;
-    else
-      return LE_GREATER;
-  }
-}
+COMPILER_RT_ABI CMP_RESULT __lesf2(fp_t a, fp_t b) { return __leXf2__(a, b); }
 
 #if defined(__ELF__)
 // Alias for libgcc compatibility
 COMPILER_RT_ALIAS(__lesf2, __cmpsf2)
 #endif
 COMPILER_RT_ALIAS(__lesf2, __eqsf2)
 COMPILER_RT_ALIAS(__lesf2, __ltsf2)
 COMPILER_RT_ALIAS(__lesf2, __nesf2)
 
-enum GE_RESULT {
-  GE_LESS = -1,
-  GE_EQUAL = 0,
-  GE_GREATER = 1,
-  GE_UNORDERED = -1 // Note: different from LE_UNORDERED
-};
-
-COMPILER_RT_ABI enum GE_RESULT __gesf2(fp_t a, fp_t b) {
-
-  const srep_t aInt = toRep(a);
-  const srep_t bInt = toRep(b);
-  const rep_t aAbs = aInt & absMask;
-  const rep_t bAbs = bInt & absMask;
-
-  if (aAbs > infRep || bAbs > infRep)
-    return GE_UNORDERED;
-  if ((aAbs | bAbs) == 0)
-    return GE_EQUAL;
-  if ((aInt & bInt) >= 0) {
-    if (aInt < bInt)
-      return GE_LESS;
-    else if (aInt == bInt)
-      return GE_EQUAL;
-    else
-      return GE_GREATER;
-  } else {
-    if (aInt > bInt)
-      return GE_LESS;
-    else if (aInt == bInt)
-      return GE_EQUAL;
-    else
-      return GE_GREATER;
-  }
-}
+COMPILER_RT_ABI CMP_RESULT __gesf2(fp_t a, fp_t b) { return __geXf2__(a, b); }
 
 COMPILER_RT_ALIAS(__gesf2, __gtsf2)
 
-COMPILER_RT_ABI int
-__unordsf2(fp_t a, fp_t b) {
-    const rep_t aAbs = toRep(a) & absMask;
-    const rep_t bAbs = toRep(b) & absMask;
-    return aAbs > infRep || bAbs > infRep;
+COMPILER_RT_ABI CMP_RESULT __unordsf2(fp_t a, fp_t b) {
+  return __unordXf2__(a, b);
 }
 
 #if defined(__ARM_EABI__)
 #if defined(COMPILER_RT_ARMHF_TARGET)
 AEABI_RTABI int __aeabi_fcmpun(fp_t a, fp_t b) { return __unordsf2(a, b); }
 #else
 COMPILER_RT_ALIAS(__unordsf2, __aeabi_fcmpun)
 #endif

compiler-rt/lib/builtins/comparetf2.c

 // NaN values.
 //
 //===----------------------------------------------------------------------===//
 
 #define QUAD_PRECISION
 #include "fp_lib.h"
 
 #if defined(CRT_HAS_128BIT) && defined(CRT_LDBL_128BIT)
-enum LE_RESULT { LE_LESS = -1, LE_EQUAL = 0, LE_GREATER = 1, LE_UNORDERED = 1 };
+#include "fp_compare_impl.inc"
 
-COMPILER_RT_ABI enum LE_RESULT __letf2(fp_t a, fp_t b) {
-
-  const srep_t aInt = toRep(a);
-  const srep_t bInt = toRep(b);
-  const rep_t aAbs = aInt & absMask;
-  const rep_t bAbs = bInt & absMask;
-
-  // If either a or b is NaN, they are unordered.
-  if (aAbs > infRep || bAbs > infRep)
-    return LE_UNORDERED;
-
-  // If a and b are both zeros, they are equal.
-  if ((aAbs | bAbs) == 0)
-    return LE_EQUAL;
-
-  // If at least one of a and b is positive, we get the same result comparing
-  // a and b as signed integers as we would with a floating-point compare.
-  if ((aInt & bInt) >= 0) {
-    if (aInt < bInt)
-      return LE_LESS;
-    else if (aInt == bInt)
-      return LE_EQUAL;
-    else
-      return LE_GREATER;
-  } else {
-    // Otherwise, both are negative, so we need to flip the sense of the
-    // comparison to get the correct result.  (This assumes a twos- or ones-
-    // complement integer representation; if integers are represented in a
-    // sign-magnitude representation, then this flip is incorrect).
-    if (aInt > bInt)
-      return LE_LESS;
-    else if (aInt == bInt)
-      return LE_EQUAL;
-    else
-      return LE_GREATER;
-  }
-}
+COMPILER_RT_ABI CMP_RESULT __letf2(fp_t a, fp_t b) { return __leXf2__(a, b); }
 
 #if defined(__ELF__)
 // Alias for libgcc compatibility
 COMPILER_RT_ALIAS(__letf2, __cmptf2)
 #endif
 COMPILER_RT_ALIAS(__letf2, __eqtf2)
 COMPILER_RT_ALIAS(__letf2, __lttf2)
 COMPILER_RT_ALIAS(__letf2, __netf2)
 
-enum GE_RESULT {
-  GE_LESS = -1,
-  GE_EQUAL = 0,
-  GE_GREATER = 1,
-  GE_UNORDERED = -1 // Note: different from LE_UNORDERED
-};
-
-COMPILER_RT_ABI enum GE_RESULT __getf2(fp_t a, fp_t b) {
-
-  const srep_t aInt = toRep(a);
-  const srep_t bInt = toRep(b);
-  const rep_t aAbs = aInt & absMask;
-  const rep_t bAbs = bInt & absMask;
-
-  if (aAbs > infRep || bAbs > infRep)
-    return GE_UNORDERED;
-  if ((aAbs | bAbs) == 0)
-    return GE_EQUAL;
-  if ((aInt & bInt) >= 0) {
-    if (aInt < bInt)
-      return GE_LESS;
-    else if (aInt == bInt)
-      return GE_EQUAL;
-    else
-      return GE_GREATER;
-  } else {
-    if (aInt > bInt)
-      return GE_LESS;
-    else if (aInt == bInt)
-      return GE_EQUAL;
-    else
-      return GE_GREATER;
-  }
-}
+COMPILER_RT_ABI CMP_RESULT __getf2(fp_t a, fp_t b) { return __geXf2__(a, b); }
 
 COMPILER_RT_ALIAS(__getf2, __gttf2)
 
-COMPILER_RT_ABI int __unordtf2(fp_t a, fp_t b) {
-  const rep_t aAbs = toRep(a) & absMask;
-  const rep_t bAbs = toRep(b) & absMask;
-  return aAbs > infRep || bAbs > infRep;
+COMPILER_RT_ABI CMP_RESULT __unordtf2(fp_t a, fp_t b) {
+  return __unordXf2__(a, b);
 }
 
 #endif

compiler-rt/lib/builtins/fp_compare_impl.inc

-//===-- lib/comparetf2.c - Quad-precision comparisons -------------*- C -*-===//
+//===-- lib/fp_compare_impl.inc - Floating-point comparison -------*- C -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
-//
-// // This file implements the following soft-float comparison routines:
-//
-//   __eqtf2   __getf2   __unordtf2
-//   __letf2   __gttf2
-//   __lttf2
-//   __netf2
-//
-// The semantics of the routines grouped in each column are identical, so there
-// is a single implementation for each, and wrappers to provide the other names.
-//
-// The main routines behave as follows:
-//
-//   __letf2(a,b) returns -1 if a < b
-//                         0 if a == b
-//                         1 if a > b
-//                         1 if either a or b is NaN
-//
-//   __getf2(a,b) returns -1 if a < b
-//                         0 if a == b
-//                         1 if a > b
-//                        -1 if either a or b is NaN
-//
-//   __unordtf2(a,b) returns 0 if both a and b are numbers
-//                           1 if either a or b is NaN
-//
-// Note that __letf2( ) and __getf2( ) are identical except in their handling of
-// NaN values.
-//
-//===----------------------------------------------------------------------===//
 
-#define QUAD_PRECISION
 #include "fp_lib.h"
 
-#if defined(CRT_HAS_128BIT) && defined(CRT_LDBL_128BIT)
-enum LE_RESULT { LE_LESS = -1, LE_EQUAL = 0, LE_GREATER = 1, LE_UNORDERED = 1 };
+// GCC uses long (at least for x86_64) as the return type of the comparison
+// functions. We need to ensure that the return value is sign-extended in the
+// same way as GCC expects (since otherwise GCC-generated __builtin_isinf
+// returns true for finite 128-bit floating-point numbers).
+#ifdef __aarch64__
+// AArch64 GCC overrides libgcc_cmp_return to use int instead of long.
+typedef int CMP_RESULT;
+#elif __SIZEOF_POINTER__ == 8 && __SIZEOF_LONG__ == 4
+// LLP64 ABIs use long long instead of long.
+typedef long long CMP_RESULT;
+#else
+// Otherwise the comparison functions return long.
+typedef long CMP_RESULT;
+#endif
 
-COMPILER_RT_ABI enum LE_RESULT __letf2(fp_t a, fp_t b) {
+#if !defined(__clang__) && defined(__GNUC__)
+// GCC uses a special __libgcc_cmp_return__ mode to define the return type, so
+// check that we are ABI-compatible when compiling the builtins with GCC.
+typedef int GCC_CMP_RESULT __attribute__((__mode__(__libgcc_cmp_return__)));
+_Static_assert(sizeof(GCC_CMP_RESULT) == sizeof(CMP_RESULT),
+               "SOFTFP ABI not compatible with GCC");
+#endif
 
+enum {
+  LE_LESS = -1,
+  LE_EQUAL = 0,
+  LE_GREATER = 1,
+  LE_UNORDERED = 1,
+};
+
+static inline CMP_RESULT __leXf2__(fp_t a, fp_t b) {
   const srep_t aInt = toRep(a);
   const srep_t bInt = toRep(b);
   const rep_t aAbs = aInt & absMask;
   const rep_t bAbs = bInt & absMask;
 
   // If either a or b is NaN, they are unordered.
   if (aAbs > infRep || bAbs > infRep)
     return LE_UNORDERED;
if (aInt > bInt)
       return LE_LESS;
     else if (aInt == bInt)
       return LE_EQUAL;
     else
       return LE_GREATER;
   }
 }
 
-#if defined(__ELF__)
-// Alias for libgcc compatibility
-COMPILER_RT_ALIAS(__letf2, __cmptf2)
-#endif
-COMPILER_RT_ALIAS(__letf2, __eqtf2)
-COMPILER_RT_ALIAS(__letf2, __lttf2)
-COMPILER_RT_ALIAS(__letf2, __netf2)
-
-enum GE_RESULT {
+enum {
   GE_LESS = -1,
   GE_EQUAL = 0,
   GE_GREATER = 1,
   GE_UNORDERED = -1 // Note: different from LE_UNORDERED
 };
 
-COMPILER_RT_ABI enum GE_RESULT __getf2(fp_t a, fp_t b) {
-
+static inline CMP_RESULT __geXf2__(fp_t a, fp_t b) {
   const srep_t aInt = toRep(a);
   const srep_t bInt = toRep(b);
   const rep_t aAbs = aInt & absMask;
   const rep_t bAbs = bInt & absMask;
 
   if (aAbs > infRep || bAbs > infRep)
     return GE_UNORDERED;
   if ((aAbs | bAbs) == 0)
if (aInt > bInt)
       return GE_LESS;
     else if (aInt == bInt)
       return GE_EQUAL;
     else
       return GE_GREATER;
   }
 }
 
-COMPILER_RT_ALIAS(__getf2, __gttf2)
-
-COMPILER_RT_ABI int __unordtf2(fp_t a, fp_t b) {
+static inline CMP_RESULT __unordXf2__(fp_t a, fp_t b) {
   const rep_t aAbs = toRep(a) & absMask;
   const rep_t bAbs = toRep(b) & absMask;
   return aAbs > infRep || bAbs > infRep;
 }
-
-#endif