[builtins] Support building the 128-bit float functions on i386 #122658

zhangtianhao6 · 2025-01-13T02:48:33Z

GCC provides these functions (e.g.__subtf3) in libgcc on i386.
Since Clang supports float128, we can also enable the existing code in i386 for ABI compatible.

#121757

github-actions · 2025-01-13T02:48:51Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

github-actions · 2025-01-13T03:52:26Z

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

You can test this locally with the following command:

git-clang-format --diff 211bcf67aadb1175af382f55403ae759177281c7 fe61e4aeae112ec6a1ed1d3d2bd0537a582eea9f --extensions c -- compiler-rt/lib/builtins/extendxftf2.c compiler-rt/lib/builtins/trunctfxf2.c compiler-rt/test/builtins/Unit/extendxftf2_test.c compiler-rt/test/builtins/Unit/trunctfxf2_test.c

View the diff from clang-format here.

diff --git a/compiler-rt/test/builtins/Unit/extendxftf2_test.c b/compiler-rt/test/builtins/Unit/extendxftf2_test.c
index e82dd67404..d5bec2b463 100644
--- a/compiler-rt/test/builtins/Unit/extendxftf2_test.c
+++ b/compiler-rt/test/builtins/Unit/extendxftf2_test.c
@@ -7,7 +7,7 @@
 #if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) &&   \
     (defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))
 
-#include "fp_test.h"
+#  include "fp_test.h"
 
 COMPILER_RT_ABI tf_float __extendxftf2(long double a);
 
diff --git a/compiler-rt/test/builtins/Unit/trunctfxf2_test.c b/compiler-rt/test/builtins/Unit/trunctfxf2_test.c
index ea88732aa7..bd195d64c4 100644
--- a/compiler-rt/test/builtins/Unit/trunctfxf2_test.c
+++ b/compiler-rt/test/builtins/Unit/trunctfxf2_test.c
@@ -7,7 +7,7 @@
 #if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) &&   \
     (defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))
 
-#include "fp_test.h"
+#  include "fp_test.h"
 
 COMPILER_RT_ABI long double __trunctfxf2(tf_float a);

phoebewang · 2025-01-13T04:31:11Z

compiler-rt/lib/builtins/extendxftf2.c

@@ -12,7 +12,7 @@
 #define QUAD_PRECISION
 #include "fp_lib.h"

-#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && defined(__x86_64__)
+#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__))


Enable __i386__ in extendxftf2_test.c

OK, enabled.

phoebewang · 2025-01-13T04:31:39Z

compiler-rt/lib/builtins/trunctfxf2.c

@@ -12,7 +12,7 @@
 #define QUAD_PRECISION
 #include "fp_lib.h"

-#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && defined(__x86_64__)
+#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__))


Enable i386 in trunctfxf2_test.c

OK, enabled.

GCC provides these functions (e.g.__subtf3) in libgcc on i386. Since Clang supports float128, we can also enable the existing code in i386 for ABI compatible.

hstk30-hw · 2025-01-13T08:22:18Z

@tru Take a look, the code_formatter suggestion is weird.

compnerd · 2025-01-13T15:44:54Z

compiler-rt/lib/builtins/extendxftf2.c

@@ -12,7 +12,8 @@
 #define QUAD_PRECISION
 #include "fp_lib.h"

-#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && defined(__x86_64__)
+#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 &&                     \


This change is concerning. Not all 32-bit environments support FP80. Concretely, Windows, FreeBSD, and Darwin definitely do support FP80 even though the hardware does. This can make it easy to get ABI breakages.

how about only enable for linux

This should actually be checking HAS_80_BIT_LONG_DOUBLE, which is set correctly depending on environments that support it:

llvm-project/compiler-rt/lib/builtins/int_types.h

Line 158 in 4a4a8a1

#if (defined(__i386__) || defined(__x86_64__)) && \

Suggested change

#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && \

#if defined(CRT_HAS_TF_MODE) && HAS_80_BIT_LONG_DOUBLE

compnerd · 2025-01-13T15:45:02Z

compiler-rt/lib/builtins/trunctfxf2.c

@@ -12,7 +12,8 @@
 #define QUAD_PRECISION
 #include "fp_lib.h"

-#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && defined(__x86_64__)
+#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 &&                     \


arichardson

Thanks this mostly looks good just a few suggestions

arichardson · 2025-01-15T22:17:34Z

compiler-rt/lib/builtins/CMakeLists.txt

@@ -906,7 +907,7 @@ else ()

      # For RISCV32, we must force enable int128 for compiling long
      # double routines.
-      if(COMPILER_RT_ENABLE_SOFTWARE_INT128 OR "${arch}" STREQUAL "riscv32")
+      if(COMPILER_RT_ENABLE_SOFTWARE_INT128 OR "${arch}" STREQUAL "riscv32" OR "${arch}" STREQUAL "i386")
        list(APPEND BUILTIN_CFLAGS_${arch} -fforce-enable-int128)


Do we support other compilers? If so enabling this unconditionally for i386 might break them?

arichardson · 2025-01-15T22:20:46Z

compiler-rt/lib/builtins/extendxftf2.c

@@ -12,7 +12,8 @@
 #define QUAD_PRECISION
 #include "fp_lib.h"

-#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && defined(__x86_64__)
+#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 &&                     \


This should actually be checking HAS_80_BIT_LONG_DOUBLE, which is set correctly depending on environments that support it:

llvm-project/compiler-rt/lib/builtins/int_types.h

Line 158 in 4a4a8a1

#if (defined(__i386__) || defined(__x86_64__)) && \

Suggested change

#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && \

#if defined(CRT_HAS_TF_MODE) && HAS_80_BIT_LONG_DOUBLE

arichardson · 2025-01-15T22:20:59Z

compiler-rt/lib/builtins/trunctfxf2.c

+#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 &&                     \
+    (defined(__x86_64__) || defined(__i386__))


Suggested change

#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && \

(defined(__x86_64__) || defined(__i386__))

#if defined(CRT_HAS_TF_MODE) && HAS_80_BIT_LONG_DOUBLE

As above

arichardson · 2025-01-15T22:22:03Z

compiler-rt/test/builtins/Unit/extendxftf2_test.c

+#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) &&   \
    (defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))


Suggested change

#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) && \

(defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))

#if defined(CRT_HAS_TF_MODE) && HAS_80_BIT_LONG_DOUBLE

Since we include int_lib.h should be possible to simplify this

arichardson · 2025-01-15T22:22:14Z

compiler-rt/test/builtins/Unit/extendxftf2_test.c

+#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) &&   \
    (defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))


Suggested change

#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) && \

(defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))

#if defined(CRT_HAS_TF_MODE) && HAS_80_BIT_LONG_DOUBLE

Since we include int_lib.h should be possible to simplify this

arichardson · 2025-01-15T22:22:22Z

compiler-rt/test/builtins/Unit/trunctfxf2_test.c

+#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) &&   \
    (defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))


Suggested change

#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) && \

(defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))

#if defined(CRT_HAS_TF_MODE) && HAS_80_BIT_LONG_DOUBLE

Since we include int_lib.h should be possible to simplify this

arichardson · 2025-01-15T22:22:31Z

compiler-rt/test/builtins/Unit/trunctfxf2_test.c

+#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) &&   \
    (defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))


Suggested change

#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) || defined(__i386__)) && \

(defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__))

#if defined(CRT_HAS_TF_MODE) && HAS_80_BIT_LONG_DOUBLE

Since we include int_lib.h should be possible to simplify this

arichardson · 2025-01-15T22:23:40Z

compiler-rt/test/builtins/Unit/trunctfxf2_test.c

@@ -1,10 +1,10 @@
-// RUN: %clang_builtins %s %librt -o %t && %run %t
+// RUN: %clang_builtins %s %librt -fforce-enable-int128 -o %t && %run %t


Will adding this flag unconditionally break other/older compilers? Maybe this needs to be a lit substitution?

llvmbot added compiler-rt compiler-rt:builtins labels Jan 13, 2025

zhangtianhao6 force-pushed the i386-support-compiler-rt-fp128-builtin branch from b1dd80a to 0fa2736 Compare January 13, 2025 03:12

hstk30-hw requested review from dongjianqiang2, phoebewang and compnerd January 13, 2025 03:48

phoebewang reviewed Jan 13, 2025

View reviewed changes

hstk30-hw requested a review from alexander-shaposhnikov January 13, 2025 06:18

zhangtianhao6 force-pushed the i386-support-compiler-rt-fp128-builtin branch 2 times, most recently from 627acf7 to 26f0383 Compare January 13, 2025 07:20

[builtins] Support building the 128-bit float functions on i386

fe61e4a

GCC provides these functions (e.g.__subtf3) in libgcc on i386. Since Clang supports float128, we can also enable the existing code in i386 for ABI compatible.

zhangtianhao6 force-pushed the i386-support-compiler-rt-fp128-builtin branch from 26f0383 to fe61e4a Compare January 13, 2025 07:29

zhangtianhao6 requested a review from phoebewang January 13, 2025 08:23

compnerd reviewed Jan 13, 2025

View reviewed changes

arichardson requested changes Jan 15, 2025

View reviewed changes

arichardson reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[builtins] Support building the 128-bit float functions on i386 #122658

[builtins] Support building the 128-bit float functions on i386 #122658

zhangtianhao6 commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 •

edited

Loading

phoebewang Jan 13, 2025

zhangtianhao6 Jan 13, 2025

phoebewang Jan 13, 2025

zhangtianhao6 Jan 13, 2025

hstk30-hw commented Jan 13, 2025

compnerd Jan 13, 2025

zhangtianhao6 Jan 14, 2025

arichardson Jan 15, 2025

compnerd Jan 13, 2025

arichardson left a comment

arichardson Jan 15, 2025

arichardson Jan 15, 2025

arichardson Jan 15, 2025

arichardson Jan 15, 2025

arichardson Jan 15, 2025

arichardson Jan 15, 2025

arichardson Jan 15, 2025

arichardson Jan 15, 2025

	#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && \
	#if defined(CRT_HAS_TF_MODE) && HAS_80_BIT_LONG_DOUBLE

		#if defined(CRT_HAS_TF_MODE) && __LDBL_MANT_DIG__ == 64 && \
		(defined(__x86_64__) \|\| defined(__i386__))

		#if __LDBL_MANT_DIG__ == 64 && (defined(__x86_64__) \|\| defined(__i386__)) && \
		(defined(__FLOAT128__) \|\| defined(__SIZEOF_FLOAT128__))

		@@ -1,10 +1,10 @@
		// RUN: %clang_builtins %s %librt -o %t && %run %t
		// RUN: %clang_builtins %s %librt -fforce-enable-int128 -o %t && %run %t

[builtins] Support building the 128-bit float functions on i386 #122658

Are you sure you want to change the base?

[builtins] Support building the 128-bit float functions on i386 #122658

Conversation

zhangtianhao6 commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hstk30-hw commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2025 •

edited

Loading