Skip to content

[libc] Fix integer rint variants on the GPU #98095

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

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Jul 8, 2024

Summary:
Currently these are implemented as a static cast on __builtin_rint()
to a long. Howver, this is not strictly correct. The standard states
that the output is unspecified, but most implementations, and the LLVM
libc implementation, do some kind of guarantee on this beahvior. This is
not guaranteed by just doing a cast. This patch just uses the generic
versions until we implement __builitin_lrint correctly.

Summary:
Currently these are implemented as a static cast on `__builtin_rint()`
to a long. Howver, this is not strictly correct. The standard states
that the output is unspecified, but most implementations, and the LLVM
libc implementation, do some kind of guarantee on this beahvior. This is
not guaranteed by just doing a cast. This patch just uses the generic
versions until we implement `__builitin_lrint` correctly.
@llvmbot
Copy link
Member

llvmbot commented Jul 8, 2024

@llvm/pr-subscribers-libc

@llvm/pr-subscribers-backend-amdgpu

Author: Joseph Huber (jhuber6)

Changes

Summary:
Currently these are implemented as a static cast on __builtin_rint()
to a long. Howver, this is not strictly correct. The standard states
that the output is unspecified, but most implementations, and the LLVM
libc implementation, do some kind of guarantee on this beahvior. This is
not guaranteed by just doing a cast. This patch just uses the generic
versions until we implement __builitin_lrint correctly.


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

4 Files Affected:

  • (modified) libc/src/math/amdgpu/CMakeLists.txt (-48)
  • (modified) libc/src/math/nvptx/CMakeLists.txt (-48)
  • (modified) libc/test/src/math/RoundToIntegerTest.h (+1)
  • (modified) libc/test/src/math/smoke/RoundToIntegerTest.h (+6-3)
diff --git a/libc/src/math/amdgpu/CMakeLists.txt b/libc/src/math/amdgpu/CMakeLists.txt
index bc81f7b20a71d..202177f54b11a 100644
--- a/libc/src/math/amdgpu/CMakeLists.txt
+++ b/libc/src/math/amdgpu/CMakeLists.txt
@@ -456,54 +456,6 @@ add_entrypoint_object(
   VENDOR
 )
 
-add_entrypoint_object(
-  lrint
-  SRCS
-    lrint.cpp
-  HDRS
-    ../lrint.h
-  COMPILE_OPTIONS
-    ${bitcode_link_flags}
-    -O2
-  VENDOR
-)
-
-add_entrypoint_object(
-  lrintf
-  SRCS
-    lrintf.cpp
-  HDRS
-    ../lrintf.h
-  COMPILE_OPTIONS
-    ${bitcode_link_flags}
-    -O2
-  VENDOR
-)
-
-add_entrypoint_object(
-  llrint
-  SRCS
-    llrint.cpp
-  HDRS
-    ../llrint.h
-  COMPILE_OPTIONS
-    ${bitcode_link_flags}
-    -O2
-  VENDOR
-)
-
-add_entrypoint_object(
-  llrintf
-  SRCS
-    llrintf.cpp
-  HDRS
-    ../llrintf.h
-  COMPILE_OPTIONS
-    ${bitcode_link_flags}
-    -O2
-  VENDOR
-)
-
 add_entrypoint_object(
   pow
   SRCS
diff --git a/libc/src/math/nvptx/CMakeLists.txt b/libc/src/math/nvptx/CMakeLists.txt
index a09668ca10678..bf37c52f09e44 100644
--- a/libc/src/math/nvptx/CMakeLists.txt
+++ b/libc/src/math/nvptx/CMakeLists.txt
@@ -409,54 +409,6 @@ add_entrypoint_object(
   VENDOR
 )
 
-add_entrypoint_object(
-  lrint
-  SRCS
-    lrint.cpp
-  HDRS
-    ../lrint.h
-  COMPILE_OPTIONS
-    ${bitcode_link_flags}
-    -O2
-  VENDOR
-)
-
-add_entrypoint_object(
-  lrintf
-  SRCS
-    lrintf.cpp
-  HDRS
-    ../lrintf.h
-  COMPILE_OPTIONS
-    ${bitcode_link_flags}
-    -O2
-  VENDOR
-)
-
-add_entrypoint_object(
-  llrint
-  SRCS
-    llrint.cpp
-  HDRS
-    ../llrint.h
-  COMPILE_OPTIONS
-    ${bitcode_link_flags}
-    -O2
-  VENDOR
-)
-
-add_entrypoint_object(
-  llrintf
-  SRCS
-    llrintf.cpp
-  HDRS
-    ../llrintf.h
-  COMPILE_OPTIONS
-    ${bitcode_link_flags}
-    -O2
-  VENDOR
-)
-
 add_entrypoint_object(
   pow
   SRCS
diff --git a/libc/test/src/math/RoundToIntegerTest.h b/libc/test/src/math/RoundToIntegerTest.h
index bb7e8643973c3..2b1c643267590 100644
--- a/libc/test/src/math/RoundToIntegerTest.h
+++ b/libc/test/src/math/RoundToIntegerTest.h
@@ -12,6 +12,7 @@
 #include "src/__support/CPP/algorithm.h"
 #include "src/__support/FPUtil/FEnvImpl.h"
 #include "src/__support/FPUtil/FPBits.h"
+#include "src/__support/macros/properties/architectures.h"
 #include "test/UnitTest/FEnvSafeTest.h"
 #include "test/UnitTest/FPMatcher.h"
 #include "test/UnitTest/Test.h"
diff --git a/libc/test/src/math/smoke/RoundToIntegerTest.h b/libc/test/src/math/smoke/RoundToIntegerTest.h
index fd3fbde92b963..8b5f678b32449 100644
--- a/libc/test/src/math/smoke/RoundToIntegerTest.h
+++ b/libc/test/src/math/smoke/RoundToIntegerTest.h
@@ -12,7 +12,6 @@
 #include "src/__support/CPP/algorithm.h"
 #include "src/__support/FPUtil/FEnvImpl.h"
 #include "src/__support/FPUtil/FPBits.h"
-#include "src/__support/macros/properties/architectures.h"
 #include "test/UnitTest/FEnvSafeTest.h"
 #include "test/UnitTest/FPMatcher.h"
 #include "test/UnitTest/Test.h"
@@ -51,12 +50,10 @@ class RoundToIntegerTestTemplate
     // 0 for errno and exceptions, but this doesn't hold for
     // all math functions using RoundToInteger test:
     // https://github.com/llvm/llvm-project/pull/88816
-#ifndef LIBC_TARGET_ARCH_IS_GPU
     if (expectError) {
       ASSERT_FP_EXCEPTION(FE_INVALID);
       ASSERT_MATH_ERRNO(EDOM);
     }
-#endif
   }
 
 public:
@@ -169,7 +166,13 @@ class RoundToIntegerTestTemplate
 #define LIST_ROUND_TO_INTEGER_TESTS(F, I, func)                                \
   LIST_ROUND_TO_INTEGER_TESTS_HELPER(F, I, func, false)
 
+// The GPU target does not support different rounding modes.
+#ifdef LIBC_TARGET_ARCH_IS_GPU
+#define LIST_ROUND_TO_INTEGER_TESTS_WITH_MODES(F, I, func)                     \
+  LIST_ROUND_TO_INTEGER_TESTS_HELPER(F, I, func, false)
+#else
 #define LIST_ROUND_TO_INTEGER_TESTS_WITH_MODES(F, I, func)                     \
   LIST_ROUND_TO_INTEGER_TESTS_HELPER(F, I, func, true)
+#endif
 
 #endif // LLVM_LIBC_TEST_SRC_MATH_SMOKE_ROUNDTOINTEGERTEST_H

@jhuber6 jhuber6 merged commit afa6bed into llvm:main Jul 9, 2024
7 of 8 checks passed
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
Summary:
Currently these are implemented as a static cast on `__builtin_rint()`
to a long. Howver, this is not strictly correct. The standard states
that the output is unspecified, but most implementations, and the LLVM
libc implementation, do some kind of guarantee on this beahvior. This is
not guaranteed by just doing a cast. This patch just uses the generic
versions until we implement `__builitin_lrint` correctly.
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.

3 participants