[libc] Create a separate proxy header for math-function-macros.h #98430

changkhothuychung · 2024-07-11T05:00:25Z

Fix #98393

llvmbot · 2024-07-11T05:01:00Z

@llvm/pr-subscribers-libc

Author: Nhat Nguyen (changkhothuychung)

Changes

Fix #98393

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

3 Files Affected:

(modified) libc/hdr/CMakeLists.txt (+7)
(added) libc/hdr/math_function_macros.h (+14)
(modified) libc/hdr/math_macros.h (-2)

diff --git a/libc/hdr/CMakeLists.txt b/libc/hdr/CMakeLists.txt
index 7549342514304..4c47f38a1ffe8 100644
--- a/libc/hdr/CMakeLists.txt
+++ b/libc/hdr/CMakeLists.txt
@@ -29,6 +29,13 @@ add_proxy_header_library(
     math_macros.h
   FULL_BUILD_DEPENDS
     libc.include.llvm-libc-macros.math_macros
+)
+
+add_proxy_header_library(
+  math_function_macros
+  HDRS
+    math_function_macros.h
+  FULL_BUILD_DEPENDS
     libc.include.math
 )
 
diff --git a/libc/hdr/math_function_macros.h b/libc/hdr/math_function_macros.h
new file mode 100644
index 0000000000000..c2de468af61b0
--- /dev/null
+++ b/libc/hdr/math_function_macros.h
@@ -0,0 +1,14 @@
+//===-- Definition of macros from math.h ----------------------------------===//
+//
+// 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_HDR_MATH_FUNCTION_MACROS_H
+#define LLVM_LIBC_HDR_MATH_FUNCTION_MACROS_H
+
+#include <math.h>
+
+#endif // LLVM_LIBC_HDR_MATH_MACROS_H
diff --git a/libc/hdr/math_macros.h b/libc/hdr/math_macros.h
index d13c5ff7647ad..135ffd498b9a0 100644
--- a/libc/hdr/math_macros.h
+++ b/libc/hdr/math_macros.h
@@ -15,8 +15,6 @@
 
 #else // Overlay mode
 
-#include <math.h>
-
 // Some older math.h header does not have FP_INT_* constants yet.
 #ifndef FP_INT_UPWARD
 #define FP_INT_UPWARD 0

lntue · 2024-07-11T16:43:28Z

libc/hdr/math_macros.h

@@ -21,7 +21,6 @@
 #if defined(__GNUC__) && !defined(__clang__)
 #define _GLIBCXX_INCLUDE_NEXT_C_HEADERS
 #endif
-#include <math.h>


<math.h> should be included in the overlay mode.

can you explain me why it is needed in overlay mode ?

If you look at other proxy headers in libc/hdr/ folder, the structure is that if it is in full build mode (detected with #ifdef LIBC_FULL_BUILD, then we include our own headers in libc/include/llvm-libc-*. OTOH, if it is in overlay mode, we will just use the corresponding system header. And in this case, the corresponding system header is <math.h>.

The main purpose of proxy headers in libc/hdr is to provide a clear separation between our own public headers and the system headers, make sure that they won't be mixed and mismatched in different build modes.

libc/hdr/CMakeLists.txt

libc/hdr/math_function_macros.h

lntue · 2024-07-12T13:52:31Z

When building the current PR's head in full build mode, some of the tests that rely on math function macros failed to build. You should update those tests to use isnan and isinf directly from FPBits class instead.

To reproduce:

$ cmake ../llvm -GNinja -DLLVM_ENABLE_PROJECTS="llvm;clang;compiler-rt;libc" -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_LIBC_FULL_BUILD=ON -DLLVM_LIBC_INCLUDE_SCUDO=ON

$ ninja libc

$ ninja check-libc

Some of the failures:

In file included from /usr/local/google/home/lntue/experiment/llvm-project/libc/test/src/math/fminf_test.cpp:9:
/usr/local/google/home/lntue/experiment/llvm-project/libc/test/src/math/FMinTest.h:68:11: error: use of undeclared identifier 'isnan'
      if (isnan(x) || isinf(x))
          ^
/usr/local/google/home/lntue/experiment/llvm-project/libc/test/src/math/fminf_test.cpp:13:1: note: in instantiation of member function 'FMinTest<float>::testRange' requested here
LIST_FMIN_TESTS(float, LIBC_NAMESPACE::fminf)
^
/usr/local/google/home/lntue/experiment/llvm-project/libc/test/src/math/FMinTest.h:90:37: note: expanded from macro 'LIST_FMIN_TESTS'
  TEST_F(LlvmLibcFMinTest, Range) { testRange(&func); }
                                    ^
/usr/local/google/home/lntue/experiment/llvm-project/libc/test/src/math/FMinTest.h:70:11: error: use of undeclared identifier 'isnan'
      if (isnan(y) || isinf(y))
          ^

changkhothuychung · 2024-07-23T03:41:57Z

FPBits

I dont see isnan and isinf in FPBits class - https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/FPBits.h

Which function are you referring to?

lntue · 2024-07-23T04:35:04Z

FPBits

I dont see isnan and isinf in FPBits class - https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/FPBits.h

Which function are you referring to?

https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/FPBits.h#L660-L672

changkhothuychung · 2024-07-28T00:59:35Z

@lntue some of the tests derive the type directly, thus I can't use the functions is_nan() and is_inf() from FPBits, do u have any recommendations to handle this? For example this test - https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/atan2f_test.cpp#L80-L81

lntue · 2024-07-28T01:03:57Z

@lntue some of the tests derive the type directly, thus I can't use the functions is_nan() and is_inf() from FPBits, do u have any recommendations to handle this? For example this test - https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/atan2f_test.cpp#L80-L81

For something like that, you can feed the floating point variables back to FPBits class without issues, like:

   if (FPBits(x).is_inf_or_nan()) ...

changkhothuychung · 2024-07-28T01:38:46Z

https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/atan2f_test.cpp#L80-L81

thanks, and how about this case ? https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/atan2f_test.cpp#L85-L87

Should I create a FPBits from result and perform is_nan and is_inf check for it?

lntue · 2024-07-28T01:42:39Z

https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/atan2f_test.cpp#L80-L81

thanks, and how about this case ? https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/atan2f_test.cpp#L85-L87

Should I create a FPBits from result and perform is_nan and is_inf check for it?

Yes, that would be the most straightforward way to do it. And for FPBits, is_inf_or_nan() is the same as is_inf() || is_nan()

changkhothuychung · 2024-07-28T02:46:53Z

@lntue do you have any idea why there are some erros with undeclared identifier? I see the file FPBits.h is included.

lntue · 2024-07-28T02:54:20Z

@lntue do you have any idea why there are some erros with undeclared identifier? I see the file FPBits.h is included.

The failure in explogxf_test is because the fully qualified name for FPBits template is LIBC_NAMESPACE::fputil::FPBits<T>, which is not defined / aliased in that test.

changkhothuychung · 2024-07-28T03:26:26Z

@lntue CI looks good, can you help me confirm the full build mode? my laptop (mac m1) has issues with building libc on full build mode.

lntue · 2024-07-28T03:37:30Z

libc/test/src/math/explogxf_test.cpp

@@ -34,7 +36,7 @@ TEST_F(LlvmLibcExplogfTest, ExpInFloatRange) {
    return static_cast<float>(result.mh * r);
  };
  auto f_check = [](float x) -> bool {
-    return !((isnan(x) || isinf(x) || x < -70 || x > 70 ||


this will also need to be fully qualified. Maybe you can have using FPBits = LIBC_NAMESPACE::fputil::FPBits<float> at the beginning of the file, and the all subsequence usage can just be FPBits

lntue

Thanks for fixing the issue!

initial attempt

8a5fa39

llvmbot added the libc label Jul 11, 2024

changkhothuychung added 2 commits July 11, 2024 01:01

Merge branch 'main' into math-proxy

9bd4b12

some fix

1b405c6

lntue self-requested a review July 11, 2024 16:42

lntue reviewed Jul 11, 2024

View reviewed changes

libc/hdr/CMakeLists.txt Show resolved Hide resolved

lntue reviewed Jul 11, 2024

View reviewed changes

libc/hdr/math_function_macros.h Show resolved Hide resolved

some fix

e765d5d

Merge branch 'main' into math-proxy

91fbeaa

fix test to use FPBits functions

a2f890b

changkhothuychung added 3 commits July 27, 2024 22:19

fix test

79c718a

fix test

7b2dfde

fix test

3480793

fix test

dcf0516

lntue reviewed Jul 28, 2024

View reviewed changes

changkhothuychung added 2 commits July 27, 2024 23:59

fix test

3a0133b

fix test

95ca96e

changkhothuychung requested a review from lntue July 28, 2024 04:20

lntue approved these changes Jul 28, 2024

View reviewed changes

lntue merged commit f8f5b17 into llvm:main Jul 28, 2024
6 checks passed

[libc] Create a separate proxy header for math-function-macros.h #98430

[libc] Create a separate proxy header for math-function-macros.h #98430

Uh oh!

Conversation

changkhothuychung commented Jul 11, 2024

Uh oh!

llvmbot commented Jul 11, 2024

Uh oh!

lntue Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

changkhothuychung Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

lntue Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lntue Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lntue commented Jul 12, 2024

Uh oh!

changkhothuychung commented Jul 23, 2024

Uh oh!

lntue commented Jul 23, 2024

Uh oh!

changkhothuychung commented Jul 28, 2024

Uh oh!

lntue commented Jul 28, 2024

Uh oh!

changkhothuychung commented Jul 28, 2024

Uh oh!

lntue commented Jul 28, 2024

Uh oh!

changkhothuychung commented Jul 28, 2024

Uh oh!

lntue commented Jul 28, 2024

Uh oh!

changkhothuychung commented Jul 28, 2024

Uh oh!

lntue Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

lntue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lntue Jul 12, 2024 •

edited

Loading