[libc] Fix generated float128 header for aarch64 target. #78017

lntue · 2024-01-13T03:23:28Z

No description provided.

llvmbot · 2024-01-13T03:24:01Z

@llvm/pr-subscribers-libc

Author: None (lntue)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/78017.diff

7 Files Affected:

(modified) libc/config/linux/api.td (+1-1)
(modified) libc/include/CMakeLists.txt (+1)
(modified) libc/include/llvm-libc-types/CMakeLists.txt (+1)
(added) libc/include/llvm-libc-types/float128.h (+30)
(modified) libc/spec/spec.td (+1-1)
(modified) libc/spec/stdc.td (+1)
(modified) libc/src/__support/macros/properties/float.h (+6-3)

diff --git a/libc/config/linux/api.td b/libc/config/linux/api.td
index 85f6b59264eb06..8d39069a96e48d 100644
--- a/libc/config/linux/api.td
+++ b/libc/config/linux/api.td
@@ -55,7 +55,7 @@ def IntTypesAPI : PublicAPI<"inttypes.h"> {
 }
 
 def MathAPI : PublicAPI<"math.h"> {
-  let Types = ["double_t", "float_t"];
+  let Types = ["double_t", "float_t", "float128"];
 }
 
 def FenvAPI: PublicAPI<"fenv.h"> {
diff --git a/libc/include/CMakeLists.txt b/libc/include/CMakeLists.txt
index 2c2d1b9b0fd155..be6668049ad4ac 100644
--- a/libc/include/CMakeLists.txt
+++ b/libc/include/CMakeLists.txt
@@ -84,6 +84,7 @@ add_gen_header(
     .llvm-libc-macros.math_macros
     .llvm-libc-types.double_t
     .llvm-libc-types.float_t
+    .llvm-libc-types.float128
 )
 
 # TODO: This should be conditional on POSIX networking being included.
diff --git a/libc/include/llvm-libc-types/CMakeLists.txt b/libc/include/llvm-libc-types/CMakeLists.txt
index 500900ffa0bbb0..7603eaac811417 100644
--- a/libc/include/llvm-libc-types/CMakeLists.txt
+++ b/libc/include/llvm-libc-types/CMakeLists.txt
@@ -96,3 +96,4 @@ add_header(rpc_opcodes_t HDR rpc_opcodes_t.h)
 add_header(ACTION HDR ACTION.h)
 add_header(ENTRY HDR ENTRY.h)
 add_header(struct_hsearch_data HDR struct_hsearch_data.h)
+add_header(float128 HDR float128.h)
diff --git a/libc/include/llvm-libc-types/float128.h b/libc/include/llvm-libc-types/float128.h
new file mode 100644
index 00000000000000..7030384e7d314a
--- /dev/null
+++ b/libc/include/llvm-libc-types/float128.h
@@ -0,0 +1,30 @@
+//===-- Definition of float128 type ---------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef __LLVM_LIBC_TYPES_FLOAT128_H__
+#define __LLVM_LIBC_TYPES_FLOAT128_H__
+
+#if defined(__clang__)
+#define LIBC_COMPILER_IS_CLANG
+#define LIBC_COMPILER_CLANG_VER (__clang_major__ * 100 + __clang_minor__)
+#elif defined(__GNUC__)
+#define LIBC_COMPILER_IS_GCC
+#define LIBC_COMPILER_GCC_VER (__GNUC__ * 100 + __GNUC_MINOR__)
+#endif
+
+#if (defined(LIBC_COMPILER_GCC_VER) && (LIBC_COMPILER_GCC_VER >= 1301)) &&     \
+    (defined(__aarch64__) || defined(__riscv) || defined(__x86_64__))
+typedef _Float128 float128;
+#elif (defined(LIBC_COMPILER_CLANG_VER) && (LIBC_COMPILER_CLANG_VER >= 600)) &&\
+    (defined(__x86_64__) && !defined(__Fuchsia__))
+typedef __float128 float128;
+#elif (LDBL_MANT_DIG == 113) || (__LDBL_MANT_DIG__ == 113)
+typedef long double float128;
+#endif
+
+#endif // __LLVM_LIBC_TYPES_FLOAT128_H__
diff --git a/libc/spec/spec.td b/libc/spec/spec.td
index 9b689b5eb502a9..5b281a496432fc 100644
--- a/libc/spec/spec.td
+++ b/libc/spec/spec.td
@@ -51,7 +51,7 @@ def LongDoubleType : NamedType<"long double">;
 def CharType : NamedType<"char">;
 
 // TODO: Add compatibility layer to use C23 type _Float128 if possible.
-def Float128Type : NamedType<"__float128">;
+def Float128Type : NamedType<"float128">;
 
 // Common types
 def VoidPtr : PtrType<VoidType>;
diff --git a/libc/spec/stdc.td b/libc/spec/stdc.td
index 714dc21f95ba54..05446b32c66da2 100644
--- a/libc/spec/stdc.td
+++ b/libc/spec/stdc.td
@@ -352,6 +352,7 @@ def StdC : StandardSpec<"stdc"> {
       [
           NamedType<"float_t">,
           NamedType<"double_t">,
+          NamedType<"float128">,
       ],
       [], // Enumerations
       [
diff --git a/libc/src/__support/macros/properties/float.h b/libc/src/__support/macros/properties/float.h
index 98ca2a5d4bc46f..bf2bcc7ffd9d63 100644
--- a/libc/src/__support/macros/properties/float.h
+++ b/libc/src/__support/macros/properties/float.h
@@ -19,11 +19,11 @@
 #include <float.h> // LDBL_MANT_DIG
 
 // 'long double' properties.
-#if (LDBL_MANT_DIG == 53)
+#if (LDBL_MANT_DIG == 53) || (__LDBL_MANT_DIG__ == 53)
 #define LIBC_LONG_DOUBLE_IS_FLOAT64
-#elif (LDBL_MANT_DIG == 64)
+#elif (LDBL_MANT_DIG == 64) || (__LDBL_MANT_DIG__ == 64)
 #define LIBC_LONG_DOUBLE_IS_X86_FLOAT80
-#elif (LDBL_MANT_DIG == 113)
+#elif (LDBL_MANT_DIG == 113) || (__LDBL_MANT_DIG__ == 113)
 #define LIBC_LONG_DOUBLE_IS_FLOAT128
 #endif
 
@@ -53,6 +53,9 @@ using float16 = _Float16;
 #endif
 
 // float128 support.
+// If the definition of float128 here is updated, also update
+//   include/llvm-libc-types/float128.h
+// so that the type definition in generated header `math.h` matched.
 #if (defined(LIBC_COMPILER_GCC_VER) && (LIBC_COMPILER_GCC_VER >= 1301)) &&     \
     (defined(LIBC_TARGET_ARCH_IS_AARCH64) ||                                   \
      defined(LIBC_TARGET_ARCH_IS_ANY_RISCV) ||                                 \

github-actions · 2024-01-13T03:26:07Z

✅ With the latest revision this PR passed the C/C++ code formatter.

nickdesaulniers · 2024-01-16T16:58:18Z

libc/include/llvm-libc-types/float128.h

+#if defined(__clang__)
+#define LIBC_COMPILER_IS_CLANG
+#define LIBC_COMPILER_CLANG_VER (__clang_major__ * 100 + __clang_minor__)
+#elif defined(__GNUC__)
+#define LIBC_COMPILER_IS_GCC
+#define LIBC_COMPILER_GCC_VER (__GNUC__ * 100 + __GNUC_MINOR__)
+#endif


This is duplicated from libc/src/__support/macros/properties/compiler.h. Can we simply include that header here?

The include/' folder is going to be the public facing when we do installing, so I don't want to leak any internal implementation symbols / dependency to it. Maybe we can export some useful macros inside src/__support/macros/` later if they are used a lot?

libc/src/__support/macros/properties/float.h

libc/include/llvm-libc-types/float128.h

michaelrj-google · 2024-01-17T22:36:37Z

This patch will now need to revert #78511

lntue · 2024-01-23T03:48:37Z

This patch will now need to revert #78511

Done.

nickdesaulniers · 2024-01-23T18:12:04Z

libc/include/llvm-libc-types/float128.h

+#if (defined(__LIBC_COMPILER_GCC_VER) && (__LIBC_COMPILER_GCC_VER >= 1301)) && \
+    (defined(__aarch64__) || defined(__riscv) || defined(__x86_64__))
+#define LIBC_COMPILER_HAS_C23_FLOAT128
+typedef _Float128 float128;
+#elif (defined(__LIBC_COMPILER_CLANG_VER) &&                                   \
+       (__LIBC_COMPILER_CLANG_VER >= 600)) &&                                  \
+    (defined(__x86_64__) && !defined(__Fuchsia__))
+#define LIBC_COMPILER_HAS_FLOAT128_EXTENSION
+typedef __float128 float128;
+#elif (LDBL_MANT_DIG == 113)
+typedef long double float128;
+#endif


GCC's preprocessor seems to define __FLT128_IS_IEC_60559__, should clang be doing so as well?

Then clang defines __FLOAT128__ ...

I wonder if we can use feature detection here, rather than version checks. Then having the messy redef then undef compiler version stuff can go away.

For example, I suspect that most of this header can simply be:

#if defined(__clang__) && defined(__FLOAT128__) typedef __float128 float128; #elif !defined(__clang__) && defined(__GNUC__) && defined(__FLT128_IS_IEC_60559__) typedef _Float128 float128; #elif (LDBL_MANT_DIG == 113; typedef long double float128; #else #error "unsupported float128 type for platform" #endif

Does IEC 60559 state what preprocessor define should exist when _Float128 is supported?

It is supposed to, but at the current stage, it's not always. See for instance: https://cpufun.substack.com/p/portable-support-for-128b-floats

nickdesaulniers · 2024-01-23T18:12:51Z

libc/include/llvm-libc-types/float128.h

+#elif (defined(__LIBC_COMPILER_CLANG_VER) &&                                   \
+       (__LIBC_COMPILER_CLANG_VER >= 600)) &&                                  \
+    (defined(__x86_64__) && !defined(__Fuchsia__))
+#define LIBC_COMPILER_HAS_FLOAT128_EXTENSION
+typedef __float128 float128;


Is there a bug filed against clang upstream that you could link to in a comment here regarding clang support for _Float128?

For instance, I suspect _Float128 is what was standardized, not __float128. It would be good to have a TODO in sources to fix this up in the future, or think about how to write it such that newer versions of clang that do support _Float128 just work.

Unfortunately, the current landscape of C23 _Float128 type is quite messy. I added a TODO for us to update this detection once clang has _Float128 type on our platforms of interest.

To my knowledge there is no principled way to test if a compiler specific type is supported.
From the standard point of view we need C++23 but LLVM libc is still C++17
https://en.cppreference.com/w/cpp/types/floating-point

Please add a link to #80195.

__STDC_IEC_60559_BFP__ and __STDC_IEC_60559_TYPES__ are what we should use for feature detection.

I use __STDC_IEC_60559_BFP__ for gcc. It looks like it's defined for gcc-11 on x86-64, aarch64, and riscv-64. __STDC_IEC_60559_TYPES__ is not defined anywhere.

Please add a link to #80195 though.

I added the link to it right next to the TODO.

nickdesaulniers · 2024-02-02T19:16:31Z

I think you should drop 76aedc8

The point of feature detection is that when any toolchain implements support and advertises that, the checks we have just work.

We should only check for advertised support of the feature, unless something is very very wrong in the compiler (in which adequate commentary on why is necessary).

nickdesaulniers · 2024-02-02T23:07:36Z

libc/include/llvm-libc-types/float128.h

+#if defined(__STDC_IEC_60559_BFP__) && !defined(__clang__)
+// Use _Float128 C23 type.
+#define LIBC_COMPILER_HAS_C23_FLOAT128
+typedef _Float128 float128;
+#elif defined(__FLOAT128__)
+// Use __float128 type.
+// clang uses __FLOAT128__ macro to notify the availability of __float128 type:
+//   https://reviews.llvm.org/D15120
+#define LIBC_COMPILER_HAS_FLOAT128_EXTENSION
+typedef __float128 float128;
+#elif (LDBL_MANT_DIG == 113)
+// Use long double.
+typedef long double float128;
+#endif


Argh, sorry for causing confusion here, but I do think we'll benefit in the long run by documenting the state of the world.

So some issues we've found:

gcc did not support _Float128 in C mode until gcc-7, and C++ mode until gcc-13.

gcc defines __STDC_IEC_60559_BFP__ to 201404L so we cannot compare __STDC_IEC_60559_BFP__ to 202311L (at least doing so won't work for any compiler TODAY). Just having this one check would be ideal, but alas.

clang doesn't support _Float128.

glibc will define __STDC_IEC_60559_BFP__ to 201404L, so we can't use it to detect compiler support in overlay mode (or not without great pain).

So perhaps we can have:

// GCC did not support _Float128 in C mode until gcc-7, or C++ mode until g++-13. #if defined(__GNUC__) && !defined(clang) && \ (defined(__cpluscplus) && __GNUC__ >= 13) || \ (!defined(__cplusplus) && __GNUC__ >= 7) #define LIBC_COMPILER_HAS_C23_FLOAT128 #endif // TODO: replace when clang has actual _Float128 support. // https://github.com/llvm/llvm-project/issues/80195 #ifdef __clang__ #define LIBC_COMPILER_HAS_FLOAT128_EXTENSION #endif #ifdef LIBC_COMPILER_HAS_C23_FLOAT128 typedef _Float128 float128; #elif defined(LIBC_COMPILER_HAS_FLOAT128_EXTENSION) typedef __float128 float128; #elif LDBL_MANT_DIG == 113 typedef long double float128; #else #error "no f128 support" #endif

I was hopefully naive that we could just have:

#if __STDC_IEC_60559_BFP__ < 202311L #error "no _Float128 support" #endif

Let me summarize the current support in the following table:

| clang | gcc pre-13 | gcc-13 | g++ pre-13 | g++-13 | ---------------------------------------------------------------------------------------------------------- | x86-64 | arm64 | x86-64 | arm64 | x86-64 | arm64 | x86-64 | arm64 | x86-64 | arm64 | ---------------------------------------------------------------------------------------------------------- _Float128 | | | X | X | X | X | | | X | X | ---------------------------------------------------------------------------------------------------------- __float128 | X | | X | | X | | X | | X | | ---------------------------------------------------------------------------------------------------------- long double as f128 | | X | | X | | X | | X | | X |

gcc arm64: https://godbolt.org/z/n5vsczs76

g++ x86-64: https://godbolt.org/z/fzqMTenzo

g++ arm64: https://godbolt.org/z/3hjqGsez5

So far the cleanest check I found without relying on compiler versioning or target architecture is follow:

#if defined(__STDC_IEC_60559_BFP__) && !defined(__clang__) && !defined(__cplusplus) #define LIBC_COMPILER_HAS_C23_FLOAT128 #elif defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__) #define LIBC_COMPILER_HAS_FLOAT128_EXTENSION #elif (LDBL_MANT_DIG == 113) // Use long double. #define LIBC_LONG_DOUBLE_IS_FLOAT128 #endif #ifdef LIBC_COMPILER_HAS_C23_FLOAT128 typedef _Float128 float128; #elif defined(LIBC_COMPILER_HAS_FLOAT128_EXTENSION) typedef __float128 float128; #elif defined(LIBC_LONG_DOUBLE_IS_FLOAT128) typedef long double float128; #endif

with comments / TODO to revisit the conditions in the future when clang has support for _Float128.
WDYT?

SGTM; we'll hopefully be revisiting this block of preprocessor checks when compilers improve their support. Let's get the build back to green.

lntue requested review from gchatelet, nickdesaulniers, jhuber6 and michaelrj-google January 13, 2024 03:23

llvmbot added the libc label Jan 13, 2024

nickdesaulniers reviewed Jan 16, 2024

View reviewed changes

lntue requested review from JDevlieghere, Endilll, nikic and a team as code owners January 19, 2024 20:40

lntue force-pushed the float128 branch from 3124191 to bc65c9c Compare January 19, 2024 20:42

lntue removed request for a team, nikic, JDevlieghere and Endilll January 19, 2024 20:43

lntue force-pushed the float128 branch from bc65c9c to 3b377aa Compare January 23, 2024 03:47

lntue requested a review from nickdesaulniers January 23, 2024 03:57

nickdesaulniers reviewed Jan 23, 2024

View reviewed changes

lntue force-pushed the float128 branch from cd6ca7a to 5733763 Compare January 25, 2024 05:40

lntue added 3 commits February 2, 2024 12:39

[libc] Fix generated float128 header for aarch64 target.

4690a6c

Fix formatting.

7e476e0

Sync and consolidate float128 type logic in one place.

e30b666

lntue added 2 commits February 2, 2024 12:40

Add missing dependency and fix __copied_hdr__ target issues.

1014ffb

Add TODO to update float128 type detection macros.

a7773b3

lntue force-pushed the float128 branch from 5733763 to a7773b3 Compare February 2, 2024 17:41

lntue added 3 commits February 2, 2024 12:55

Simplify float128 type detection macros.

0c53bce

Fix ifdef.

503687a

Exclude clang from using C23 _Float128 type.

76aedc8

nickdesaulniers reviewed Feb 2, 2024

View reviewed changes

Update float128 detection macros.

b66c904

nickdesaulniers approved these changes Feb 5, 2024

View reviewed changes

lntue merged commit d4ef4b8 into llvm:main Feb 5, 2024
4 checks passed

lntue deleted the float128 branch February 5, 2024 12:44

agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024

[libc] Fix generated float128 header for aarch64 target. (llvm#78017)

c42f2fb

lntue mentioned this pull request Mar 14, 2024

[clang][AArch64] Enable fp128 for aarch64 linux target #85070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc] Fix generated float128 header for aarch64 target. #78017

[libc] Fix generated float128 header for aarch64 target. #78017

lntue commented Jan 13, 2024

llvmbot commented Jan 13, 2024

github-actions bot commented Jan 13, 2024 •

edited

nickdesaulniers Jan 16, 2024

nickdesaulniers Jan 23, 2024

lntue Jan 23, 2024

michaelrj-google commented Jan 17, 2024

lntue commented Jan 23, 2024

nickdesaulniers Jan 23, 2024

nickdesaulniers Jan 23, 2024

lntue Jan 23, 2024

nickdesaulniers Jan 23, 2024

lntue Jan 23, 2024

lntue Jan 23, 2024

gchatelet Feb 2, 2024

nickdesaulniers Feb 2, 2024

lntue Feb 2, 2024

nickdesaulniers Feb 2, 2024

lntue Feb 2, 2024

nickdesaulniers commented Feb 2, 2024

nickdesaulniers Feb 2, 2024 •

edited

lntue Feb 3, 2024 •

edited

nickdesaulniers Feb 5, 2024

[libc] Fix generated float128 header for aarch64 target. #78017

[libc] Fix generated float128 header for aarch64 target. #78017

Conversation

lntue commented Jan 13, 2024

llvmbot commented Jan 13, 2024

github-actions bot commented Jan 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelrj-google commented Jan 17, 2024

lntue commented Jan 23, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdesaulniers commented Feb 2, 2024

nickdesaulniers Feb 2, 2024 • edited

Choose a reason for hiding this comment

lntue Feb 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2024 •

edited

nickdesaulniers Feb 2, 2024 •

edited

lntue Feb 3, 2024 •

edited