Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[builtins] Support building the 128-bit float functions on ld80 platforms #68132

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

arichardson
Copy link
Member

GCC provides these functions (e.g. __addtf3, etc.) in libgcc on x86_64. Since Clang supports float128, we can also enable the existing code by using float128 for fp_t if either FLOAT128 or SIZEOF_FLOAT128 is defined instead of only supporting these builtins for platforms with 128-bit IEEE long doubles.
This commit defines a new tf_float typedef that matches a float with attribute((mode(TF)) on each given architecture.

There are more tests that could be enabled for x86, but to keep the diff smaller, I restricted test changes to ones that started failing as part of this refactoring.

This change has been tested on x86 (natively) and aarch64,powerpc64,riscv64 and sparc64 via qemu-user.

This supersedes https://reviews.llvm.org/D98261 and should also cover the changes from #68041.

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff e40b2d40a4404e756a401fa639d466550aa419fb 3518654bd8df736eee931de59d0914ab56232cab -- compiler-rt/lib/builtins/divtc3.c compiler-rt/lib/builtins/extenddftf2.c compiler-rt/lib/builtins/extendhftf2.c compiler-rt/lib/builtins/extendsftf2.c compiler-rt/lib/builtins/extendxftf2.c compiler-rt/lib/builtins/fp_extend.h compiler-rt/lib/builtins/fp_lib.h compiler-rt/lib/builtins/fp_trunc.h compiler-rt/lib/builtins/int_math.h compiler-rt/lib/builtins/int_to_fp.h compiler-rt/lib/builtins/int_types.h compiler-rt/lib/builtins/multc3.c compiler-rt/lib/builtins/powitf2.c compiler-rt/lib/builtins/trunctfdf2.c compiler-rt/lib/builtins/trunctfhf2.c compiler-rt/lib/builtins/trunctfsf2.c compiler-rt/lib/builtins/trunctfxf2.c compiler-rt/test/builtins/Unit/addtf3_test.c compiler-rt/test/builtins/Unit/compiler_rt_fmaxl_test.c compiler-rt/test/builtins/Unit/compiler_rt_logbl_test.c compiler-rt/test/builtins/Unit/compiler_rt_scalbnl_test.c compiler-rt/test/builtins/Unit/divtc3_test.c compiler-rt/test/builtins/Unit/divtf3_test.c compiler-rt/test/builtins/Unit/extendxftf2_test.c compiler-rt/test/builtins/Unit/floatditf_test.c compiler-rt/test/builtins/Unit/floattitf_test.c compiler-rt/test/builtins/Unit/floatunditf_test.c compiler-rt/test/builtins/Unit/floatunsitf_test.c compiler-rt/test/builtins/Unit/floatuntitf_test.c compiler-rt/test/builtins/Unit/fp_test.h compiler-rt/test/builtins/Unit/trunctfxf2_test.c
View the diff from clang-format here.
diff --git a/compiler-rt/test/builtins/Unit/fp_test.h b/compiler-rt/test/builtins/Unit/fp_test.h
index 4eb54b7425c1..ccf64090f09c 100644
--- a/compiler-rt/test/builtins/Unit/fp_test.h
+++ b/compiler-rt/test/builtins/Unit/fp_test.h
@@ -42,10 +42,10 @@ static inline double fromRep64(uint64_t x)
 
 #if defined(CRT_HAS_TF_MODE)
 static inline tf_float fromRep128(uint64_t hi, uint64_t lo) {
-    __uint128_t x = ((__uint128_t)hi << 64) + lo;
-    tf_float ret;
-    memcpy(&ret, &x, 16);
-    return ret;
+  __uint128_t x = ((__uint128_t)hi << 64) + lo;
+  tf_float ret;
+  memcpy(&ret, &x, 16);
+  return ret;
 }
 #endif
 
@@ -76,9 +76,9 @@ static inline uint64_t toRep64(double x)
 
 #if defined(CRT_HAS_TF_MODE)
 static inline __uint128_t toRep128(tf_float x) {
-    __uint128_t ret;
-    memcpy(&ret, &x, 16);
-    return ret;
+  __uint128_t ret;
+  memcpy(&ret, &x, 16);
+  return ret;
 }
 #endif
 
@@ -142,21 +142,21 @@ static inline int compareResultD(double result,
 // because 128-bit integer constant can't be assigned directly
 static inline int compareResultF128(tf_float result, uint64_t expectedHi,
                                     uint64_t expectedLo) {
-    __uint128_t rep = toRep128(result);
-    uint64_t hi = rep >> 64;
-    uint64_t lo = rep;
-
-    if (hi == expectedHi && lo == expectedLo) {
-        return 0;
+  __uint128_t rep = toRep128(result);
+  uint64_t hi = rep >> 64;
+  uint64_t lo = rep;
+
+  if (hi == expectedHi && lo == expectedLo) {
+    return 0;
+  }
+  // test other possible NaN representation(signal NaN)
+  else if (expectedHi == 0x7fff800000000000UL && expectedLo == 0x0UL) {
+    if ((hi & 0x7fff000000000000UL) == 0x7fff000000000000UL &&
+        ((hi & 0xffffffffffffUL) > 0 || lo > 0)) {
+      return 0;
     }
-    // test other possible NaN representation(signal NaN)
-    else if (expectedHi == 0x7fff800000000000UL && expectedLo == 0x0UL) {
-        if ((hi & 0x7fff000000000000UL) == 0x7fff000000000000UL &&
-            ((hi & 0xffffffffffffUL) > 0 || lo > 0)) {
-            return 0;
-        }
-    }
-    return 1;
+  }
+  return 1;
 }
 #endif
 
@@ -269,7 +269,7 @@ static inline long double makeInf80(void) {
 
 #if defined(CRT_HAS_TF_MODE)
 static inline tf_float makeQNaN128(void) {
-    return fromRep128(0x7fff800000000000UL, 0x0UL);
+  return fromRep128(0x7fff800000000000UL, 0x0UL);
 }
 #endif
 
@@ -290,7 +290,7 @@ static inline double makeNaN64(uint64_t rand)
 
 #if defined(CRT_HAS_TF_MODE)
 static inline tf_float makeNaN128(uint64_t rand) {
-    return fromRep128(0x7fff000000000000UL | (rand & 0xffffffffffffUL), 0x0UL);
+  return fromRep128(0x7fff000000000000UL | (rand & 0xffffffffffffUL), 0x0UL);
 }
 #endif
 
@@ -321,10 +321,10 @@ static inline double makeNegativeInf64(void)
 
 #if defined(CRT_HAS_TF_MODE)
 static inline tf_float makeInf128(void) {
-    return fromRep128(0x7fff000000000000UL, 0x0UL);
+  return fromRep128(0x7fff000000000000UL, 0x0UL);
 }
 
 static inline tf_float makeNegativeInf128(void) {
-    return fromRep128(0xffff000000000000UL, 0x0UL);
+  return fromRep128(0xffff000000000000UL, 0x0UL);
 }
 #endif

@arichardson arichardson force-pushed the compiler-rt-tf-funcs-on-x86 branch from 58742dd to 5ed8d6f Compare October 3, 2023 17:43
@@ -6,7 +6,9 @@
#include <stdio.h>
#include "fp_lib.h"

#if defined(CRT_HAS_TF_MODE)
// TODO: This test currently only works if long double is the same as fp_t.
// On x86_64 we get undefined references to __trunctfxf2 and __extendxftf2
Copy link
Collaborator

@alexander-shaposhnikov alexander-shaposhnikov Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jfyi - I'm working on __trunctfxf2 and __extendxftf2 (#66918)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can drop this part of the diff if your change lands first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's merged now. You can drop this part

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the tests fail if I enable them. Will need to debug this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have more insights on these failures? extend and trunc functions that got introduced in #66918 may benefit from more testing. So there's a chance that these failures mean some bugs in this newly introduced extend/trunc implementations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a mismatch in the upper bits of the extended value in at least one test case, I will try to get the log later today.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ifdefs to skip the tests that can't work with non-IEEE 128 tf types

@arichardson
Copy link
Member Author

ping?

@arichardson arichardson force-pushed the compiler-rt-tf-funcs-on-x86 branch from 5ed8d6f to 96a6100 Compare October 13, 2023 23:35
@rprichard
Copy link
Contributor

Aside: x86-64 Android uses a IEEE128 (binary128) long double, and I think it's already using compiler-rt's __divtc3 / __divtf3 functions.

@arichardson arichardson changed the title [builtins] Support building the 128-bit float functions on x86 [builtins] Support building the 128-bit float functions on ld80 platforms Oct 14, 2023
@arichardson
Copy link
Member Author

arichardson commented Oct 14, 2023

Aside: x86-64 Android uses a IEEE128 (binary128) long double, and I think it's already using compiler-rt's __divtc3 / __divtf3 functions.

Ah that's interesting, was not aware of that. Updated the title to "ld80 platforms" instead. They should already work for platforms that use ieee128 long doubles.

@pranavk pranavk self-requested a review October 16, 2023 17:21
@@ -6,7 +6,9 @@
#include <stdio.h>
#include "fp_lib.h"

#if defined(CRT_HAS_TF_MODE)
// TODO: This test currently only works if long double is the same as fp_t.
// On x86_64 we get undefined references to __trunctfxf2 and __extendxftf2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's merged now. You can drop this part

compiler-rt/test/builtins/Unit/addtf3_test.c Outdated Show resolved Hide resolved
compiler-rt/test/builtins/Unit/floattitf_test.c Outdated Show resolved Hide resolved
@pranavk
Copy link
Contributor

pranavk commented Oct 17, 2023

Looks mostly good to me. Some tweaks are required after @alexander-shaposhnikov PR got merged: enabling tests, refactoring, etc.

@arichardson arichardson force-pushed the compiler-rt-tf-funcs-on-x86 branch from 96a6100 to c941112 Compare October 17, 2023 21:42
GCC provides these functions (e.g. __addtf3, etc.) in libgcc on x86_64.
Since Clang supports float128, we can also enable the existing code by
using float128 for fp_t if either __FLOAT128__ or __SIZEOF_FLOAT128__
is defined instead of only supporting these builtins for platforms with
128-bit IEEE long doubles.
This commit defines a new tf_float typedef that matches a float with
attribute((mode(TF)) on each given architecture.

There are more tests that could be enabled for x86, but to keep the diff
smaller, I restricted test changes to ones that started failing as part of
this refactoring.

This change has been tested on x86 (natively) and aarch64,powerpc64,riscv64
and sparc64 via qemu-user.

This supersedes https://reviews.llvm.org/D98261.
@arichardson arichardson force-pushed the compiler-rt-tf-funcs-on-x86 branch from 5482ddd to 3518654 Compare October 20, 2023 16:57
@arichardson
Copy link
Member Author

Rebased and re-ran the testsuite. Should be ready to land now. If there are no further objections I plan to land this PR early next week.

@pranavk
Copy link
Contributor

pranavk commented Oct 20, 2023

Sounds like a plan. Thanks for working on it.

@alexander-shaposhnikov
Copy link
Collaborator

sounds good, thanks!

@arichardson arichardson merged commit d2ce3e9 into llvm:main Oct 24, 2023
@arichardson arichardson deleted the compiler-rt-tf-funcs-on-x86 branch October 24, 2023 16:32
@perry-ca
Copy link
Contributor

perry-ca commented Jan 4, 2024

Richard, I've discovered that this change regresses a build we have for the z/os platform. It might impact others too. The problem we've discovered is that __multc3 is no longer being defined for our builds. We have QUAD_PRECISION but don't have TF_MODE. This change is making multc3.cc empty unless CRT_HAS_TF_MODE is defined. That breaks z/os and I suspect some other platforms.

This same problem impacts __divtc3. I'm not sure about the entire scope yet.

@arichardson
Copy link
Member Author

@perry-ca Could you give me a dump of the compiler predefined macros? The builtins headers should define CRT_HAS_TF_MODE if you have both 128-bit floats and 128-bit integers.

@perry-ca
Copy link
Contributor

perry-ca commented Jan 4, 2024 via email

@arichardson
Copy link
Member Author

arichardson commented Jan 4, 2024

It sounds to me like the easiest way forward here would be to add another #elif __FLT_RADIX__ == 16 && __LDBL_MANT_DIG__ == 28 just before #elif __LDBL_MANT_DIG__ == 113 that defines the require typedefs and macros in compiler-rt/lib/builtins/int_types.h?

@perry-ca
Copy link
Contributor

perry-ca commented Jan 4, 2024 via email

@arichardson
Copy link
Member Author

@perry-ca Could you open this as a pull request? The current diff is quite hard to read. I'm not sure about the changes to compiler-rt/lib/builtins/fp_lib.h but otherwise I think it looks sensible.

@perry-ca
Copy link
Contributor

I've created #77554 with the changes. Please review. Thanks

@rorth
Copy link
Collaborator

rorth commented Jan 10, 2024

Issue #47073 was recently marked as fixed. However, when looking at one of the affected tests (addtf3_test.c), I noticed that the test just emits "skipped". This is no wonder since the test tests CRT_HAS_IEEE_TF before including (directly or indirectly) a header that defines this (int_types.h).

AFAICS addtf3_test.c is the only one, but I only checked the tests affected by Issue #47073.

arichardson added a commit that referenced this pull request Jan 12, 2024
#68132 ended up removing
__multc3 & __divtc3 from compiler-rt library builds that have
QUAD_PRECISION but not TF_MODE due to missing int128 support. I added support for QUAD_PRECISION to
use the native hex float long double representation.

---------

Co-authored-by: Alexander Richardson <mail@alexrichardson.me>
@perry-ca
Copy link
Contributor

perry-ca commented Jan 16, 2024 via email

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
llvm#68132 ended up removing
__multc3 & __divtc3 from compiler-rt library builds that have
QUAD_PRECISION but not TF_MODE due to missing int128 support. I added support for QUAD_PRECISION to
use the native hex float long double representation.

---------

Co-authored-by: Alexander Richardson <mail@alexrichardson.me>
arichardson added a commit that referenced this pull request Feb 21, 2024
This re-lands cc0065a in a way that 
keeps existing targets working.

---------

Original commit message:
#68132 ended up removing
__multc3 & __divtc3 from compiler-rt library builds that have
QUAD_PRECISION but not TF_MODE due to missing int128 support. 
I added support for QUAD_PRECISION to use the native hex float long double representation.

---------

Co-authored-by: Sean Perry <perry@ca.ibm.com>
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 25, 2024
This re-lands cc0065a in a way that
keeps existing targets working.

---------

Original commit message:
llvm#68132 ended up removing
__multc3 & __divtc3 from compiler-rt library builds that have
QUAD_PRECISION but not TF_MODE due to missing int128 support.
I added support for QUAD_PRECISION to use the native hex float long double representation.

---------

Co-authored-by: Sean Perry <perry@ca.ibm.com>
(cherry picked from commit 99c457d)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 13, 2024
This re-lands cc0065a in a way that
keeps existing targets working.

---------

Original commit message:
llvm#68132 ended up removing
__multc3 & __divtc3 from compiler-rt library builds that have
QUAD_PRECISION but not TF_MODE due to missing int128 support.
I added support for QUAD_PRECISION to use the native hex float long double representation.

---------

Co-authored-by: Sean Perry <perry@ca.ibm.com>
(cherry picked from commit 99c457d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants