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

[libc][math][c23] Temporarily disable float16 on 32-bit Arm #95027

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

overmighty
Copy link
Member

@llvmbot llvmbot added the libc label Jun 10, 2024
@overmighty
Copy link
Member Author

cc @lntue

@llvmbot
Copy link
Member

llvmbot commented Jun 10, 2024

@llvm/pr-subscribers-libc

Author: OverMighty (overmighty)

Changes

See Buildbot failure: https://lab.llvm.org/buildbot/#/builders/229/builds/27009.


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

1 Files Affected:

  • (modified) libc/include/llvm-libc-macros/float16-macros.h (+1-1)
diff --git a/libc/include/llvm-libc-macros/float16-macros.h b/libc/include/llvm-libc-macros/float16-macros.h
index 3f819ad53df71..e7d8d93bca1b6 100644
--- a/libc/include/llvm-libc-macros/float16-macros.h
+++ b/libc/include/llvm-libc-macros/float16-macros.h
@@ -11,7 +11,7 @@
 
 #if defined(__FLT16_MANT_DIG__) &&                                             \
     (!defined(__GNUC__) || __GNUC__ >= 13 || defined(__clang__)) &&            \
-    !defined(__riscv)
+    !defined(__arm__) && !defined(_M_ARM) && !defined(__riscv)
 #define LIBC_TYPES_HAS_FLOAT16
 #endif
 

@lntue lntue merged commit a9e5f42 into llvm:main Jun 10, 2024
6 of 7 checks passed
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
@nickdesaulniers
Copy link
Member

What's the plan for getting this re-enabled? #94807 ?

@overmighty
Copy link
Member Author

@nickdesaulniers We have a few ideas that aren't incompatible:

  • Writing our own functions to convert between _Float16 and higher-precision types with bit manipulation, to not depend on the functions from the compiler runtime.
  • Using inline assembly for targets that support instructions for converting between _Float16 and higher-precision types, to not depend on the functions from the compiler runtime.
  • Linking with the latest compiler-rt (I haven't yet confirmed that it fixes the issue). It could be an option for the runtime build.

@nickdesaulniers
Copy link
Member

nickdesaulniers commented Jun 26, 2024

I'm always in favor of not requiring a dependency on a compiler runtime, though llvm-libc is NOT capable of that at this point. Here's a build of llvm-libc for 32b ARM:

$ llvm-nm libc/lib/libc.a | llvm-cxxfilt | grep aeabi
         U __aeabi_read_tp
         U __aeabi_ldivmod
         U __aeabi_uldivmod
         U __aeabi_ul2d
         U __aeabi_idiv
         U __aeabi_idiv
         U __aeabi_ldivmod
         U __aeabi_ul2d
         U __aeabi_uidiv
         U __aeabi_uidiv
         U __aeabi_ul2d
         U __aeabi_uldivmod
         U __aeabi_uidiv
         U __aeabi_uidivmod

Those symbols will be satisfied by the compiler runtime.

From a quick discussion in #gcc on oftc.irc.net, it sounds like half precision ops are promoted to single precision for soft float.


That buildbot link above isn't loading for me; do you recall what the error was, specifically?

@overmighty
Copy link
Member Author

That buildbot link above isn't loading for me; do you recall what the error was, specifically?

It sends a request that 404's. After a recent Buildbot maintenance, builder IDs changed and we also lost all build logs.

I believe this was the error:

FAILED: projects/libc/test/src/math/libc.test.src.math.cosf_test.__unit__.__build__ 
: && /usr/bin/clang++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -g  projects/libc/test/src/math/CMakeFiles/libc.test.src.math.cosf_test.__unit__.__build__.dir/cosf_test.cpp.o -o projects/libc/test/src/math/libc.test.src.math.cosf_test.__unit__.__build__  projects/libc/src/errno/CMakeFiles/libc.src.errno.errno.__internal__.dir/./libc_errno.cpp.o  projects/libc/src/__support/OSUtil/linux/CMakeFiles/libc.src.__support.OSUtil.linux.linux_util.dir/./exit.cpp.o  projects/libc/src/math/generic/CMakeFiles/libc.src.math.generic.cosf.__internal__.dir/./cosf.cpp.o  projects/libc/src/__support/StringUtil/CMakeFiles/libc.src.__support.StringUtil.error_to_string.dir/./error_to_string.cpp.o  lib/liblibcMPFRWrapper.a  -lmpfr  -lgmp  lib/libLibcFPTestHelpers.unit.a  lib/libLibcDeathTestExecutors.unit.a  lib/libLibcTest.unit.a  -lmpfr  -lgmp && :
/usr/bin/arm-linux-gnueabihf-ld: lib/liblibcMPFRWrapper.a(MPFRUtils.cpp.o): in function `_Float16 __llvm_libc_19_0_0_git::testing::mpfr::MPFRNumber::as<_Float16>() const':
/llvm/libc_worker/worker/libc-arm32-debian/libc-arm32-debian-dbg/llvm-project/libc/utils/MPFRWrapper/MPFRUtils.cpp:606: undefined reference to `__aeabi_d2h'
/usr/bin/arm-linux-gnueabihf-ld: lib/liblibcMPFRWrapper.a(MPFRUtils.cpp.o): in function `void __llvm_libc_19_0_0_git::testing::mpfr::internal::explain_unary_operation_single_output_error<_Float16>(__llvm_libc_19_0_0_git::testing::mpfr::Operation, _Float16, _Float16, double, __llvm_libc_19_0_0_git::fputil::testing::RoundingMode)':
/llvm/libc_worker/worker/libc-arm32-debian/libc-arm32-debian-dbg/llvm-project/libc/utils/MPFRWrapper/MPFRUtils.cpp:606: undefined reference to `__aeabi_d2h'
/usr/bin/arm-linux-gnueabihf-ld: lib/liblibcMPFRWrapper.a(MPFRUtils.cpp.o): in function `_Float16 __llvm_libc_19_0_0_git::testing::mpfr::MPFRNumber::as<_Float16>() const':
/llvm/libc_worker/worker/libc-arm32-debian/libc-arm32-debian-dbg/llvm-project/libc/utils/MPFRWrapper/MPFRUtils.cpp:606: undefined reference to `__aeabi_d2h'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

It was due to these changes:

https://github.com/llvm/llvm-project/pull/94383/files#diff-fddc5593e05324c6f1d04aac0c1b47cb5ebd02d8ab21e58c85e17f124ed5f963R602-R609

@nickdesaulniers
Copy link
Member

Strange, __aeabi_d2h is used by compiler-rt to convert double to short for aarch32. I wonder if clang has a codegen bug, where it's lowering such conversions to libcalls to __aeabi_d2h, which then doesn't exist in libgcc? cc @smithp35

@smithp35
Copy link
Collaborator

smithp35 commented Oct 11, 2024

__aeabi_d2h is defined in the RTABI https://github.com/ARM-software/abi-aa/blob/main/rtabi32/rtabi32.rst#standard-conversions-between-floating-types . Strictly, but unhelpfully, speaking it is a bug that libgcc doesn't define it.

I can refer to this to our GNU team, but it even if fixed it would take some years to get into distros.

Looks like:
__aeabi_h2f -> __gnu_h2f_ieee
__aeabi_f2h -> __gnu_f2h_ieee
__aeabi_f2h -> __gnu_d2h_ieee

Which it looks like compiler-rt defines. Perhaps for linux targets clang could be changed to use these instead.

@nickdesaulniers
Copy link
Member

Possibly related to GCC's ARM port not yet supporting _Float16 type?
https://godbolt.org/z/v4brh7Gj3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants