Page MenuHomePhabricator

[compiler-rt][builtins] Skip building (b)float16 support on i386-freebsd
ClosedPublic

Authored by dim on Oct 16 2022, 11:27 AM.

Details

Summary

Since bfloat16 and float16 support is not available for i386-freebsd,
the truncdfbf2.c and truncsfbf2.c builtin sources should be skipped
when targeting that platform, and COMPILER_RT_HAS_FLOAT16 should not
be defined.

However, the CMake configuration stage runs its tests with the default
target, which normally is amd64-freebsd, so it will detect both bfloat16
and float16 support.

Move adding of the COMPILER_RT_HAS_FLOAT16 define to the foreach()
loop where all the supported architectures are handled, and do not
enable it when targeting i386-freebsd.

Also remove the bfloat16 sources from the i386_SOURCES list, when
targeting i386-freebsd.

Diff Detail

Event Timeline

dim created this revision.Oct 16 2022, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2022, 11:27 AM
dim requested review of this revision.Oct 16 2022, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2022, 11:27 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
dim added a subscriber: brooks.Oct 16 2022, 11:30 AM

@brooks btw didn't you encounter this problem during llvm-devel port builds ? E.g. it dies with:

In file included from /share/dim/src/llvm/release-15/compiler-rt/lib/builtins/truncsfbf2.c:11:
In file included from /share/dim/src/llvm/release-15/compiler-rt/lib/builtins/fp_trunc_impl.inc:39:
/share/dim/src/llvm/release-15/compiler-rt/lib/builtins/fp_trunc.h:63:9: error: __bf16 is not supported on this target
typedef __bf16 dst_t;
        ^

and similar errors for the float16 support.

dim added a comment.Oct 16 2022, 11:33 AM

Another note to clarify: the problem is that the builtins are built *twice*, once for x86_64 and once for i386, the latter using the -m32 compiler flag. In the CMake configuration phase however, the feature detection is always done with the default compiler flags, i.e. without -m32, and therefore it will find features that aren't supported for 32-bit targets.

(I'm not sure how this problem could be solved in a more general manner, except by re-running the whole CMake configuration logic for each and every separate target architecture, using the compiler flags to enable that architecture. You would also have to have per-target config.h files, and the like...)

What is being done for things like 64bit atomics and int128 support? Surely that will have the same problem?

It sounds to me like the configure check should be redone with -m32 instead of adding an os-specific workaround

dim added a comment.EditedOct 16 2022, 3:17 PM

What is being done for things like 64bit atomics and int128 support? Surely that will have the same problem?

These are simply done inline at compile time, similar to what I tried to do in rGce6d40f5c23923a807388c58b82b4c343eced0ce (but that was unfortunately reverted).

For 64 bit atomics, compiler-rt/lib/builtins/atomic.c has:

/// Macros for determining whether a size is lock free.
#define ATOMIC_ALWAYS_LOCK_FREE_OR_ALIGNED_LOCK_FREE(size, p)                  \
  (__atomic_always_lock_free(size, p) ||                                       \
   (__atomic_always_lock_free(size, 0) && ((uintptr_t)p % size) == 0))
#define IS_LOCK_FREE_1(p) ATOMIC_ALWAYS_LOCK_FREE_OR_ALIGNED_LOCK_FREE(1, p)
#define IS_LOCK_FREE_2(p) ATOMIC_ALWAYS_LOCK_FREE_OR_ALIGNED_LOCK_FREE(2, p)
#define IS_LOCK_FREE_4(p) ATOMIC_ALWAYS_LOCK_FREE_OR_ALIGNED_LOCK_FREE(4, p)
#define IS_LOCK_FREE_8(p) ATOMIC_ALWAYS_LOCK_FREE_OR_ALIGNED_LOCK_FREE(8, p)
#define IS_LOCK_FREE_16(p) ATOMIC_ALWAYS_LOCK_FREE_OR_ALIGNED_LOCK_FREE(16, p)

and then specific functions are enabled or not depending on the value of the various IS_LOCK_FREE macros.

For int128 support there is in compiler-rt/lib/builtins/int_types.h:

#if defined(__LP64__) || defined(__wasm__) || defined(__mips64) ||             \
    defined(__riscv) || defined(_WIN64)
#define CRT_HAS_128BIT
#endif

and all instances of int128 support are fenced by CRT_HAS_128BIT. (Something similar is done for 128 bit long doubles.)

It sounds to me like the configure check should be redone with -m32 instead of adding an os-specific workaround

I don't understand enough about compiler-rt's build system (and CMake in general) to do that.

D114473 removed support for -m32 for the other runtimes - I feel like compiler-rt should probably do the same thing as well since fixing all of these assumptions that "-m32 supports the same features" would require prefixing every check.

D114473 removed support for -m32 for the other runtimes - I feel like compiler-rt should probably do the same thing as well since fixing all of these assumptions that "-m32 supports the same features" would require prefixing every check.

Sounds good.

dim added a comment.Sun, Jan 22, 1:07 PM

Btw these changes are independent of whether -m32 is used or not, they only check ${arch}. So if compiler-rt is ever changed to go over all support arches with e.g. -target x-y-z, this change would still work as-is. I'm mostly interested in getting the out-of-the-box build going on FreeBSD again, as it has been broken since 15.0.0.

Note also that the changes in D114473 seem to only have an effect on libc++, so they won't affect this change either.

If there are no more objections, I will go and and commit this now.

dim updated this revision to Diff 491202.Sun, Jan 22, 1:15 PM

Rebase onto main.

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Jan 22, 1:17 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
dim added a comment.Mon, Jan 23, 10:11 AM

Ugh, this is really a messy situation. COMPILER_RT_HAS_FLOAT16 (as a CMake feature) is also used in compiler-rt/test/builtins/CMakeLists.txt, where it is unconditionally used for each arch. I received some failures in buildbots, e.g.:

And these fail either with something similar to:

******************** TEST 'Builtins-aarch64-linux :: trunctfhf2_test.c' FAILED ********************
Script:
--
: 'RUN: at line 1';       /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1/./bin/clang   -gline-tables-only   -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta  -DCOMPILER_RT_HAS_FLOAT16  -fno-builtin -I /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/compiler-rt/lib/builtins -nodefaultlibs /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/compiler-rt/test/builtins/Unit/trunctfhf2_test.c /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1/./lib/clang/16/lib/aarch64-unknown-linux-gnu/libclang_rt.builtins.a -lc -lm -o /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1/projects/compiler-rt/test/builtins/Unit/AARCH64LinuxConfig/Output/trunctfhf2_test.c.tmp &&  /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1/projects/compiler-rt/test/builtins/Unit/AARCH64LinuxConfig/Output/trunctfhf2_test.c.tmp
--
Exit Code: 1

Command Output (stderr):
--
/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25
/tmp/lit-tmp-o2gvd3a7/trunctfhf2_test-59b745.o: in function `test__trunctfhf2':
trunctfhf2_test.c:(.text+0x18): undefined reference to `__trunctfhf2'
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

--

********************

or with an unexpected value:

******************** TEST 'Builtins-armhf-linux :: truncdfhf2_test.c' FAILED ********************
Script:
--
: 'RUN: at line 1';       C:/buildbot/as-builder-1/x-armv7l/build/./bin/clang.exe   -gline-tables-only -gcodeview -gcolumn-info   -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta  -fomit-frame-pointer -DCOMPILER_RT_ARMHF_TARGET -DCOMPILER_RT_HAS_FLOAT16  -fno-builtin -I C:/buildbot/as-builder-1/x-armv7l/llvm-project/compiler-rt\lib\builtins -nodefaultlibs C:\buildbot\as-builder-1\x-armv7l\llvm-project\compiler-rt\test\builtins\Unit\truncdfhf2_test.c C:/buildbot/as-builder-1/x-armv7l/build/./lib/clang/16/lib/armv7-unknown-linux-gnueabihf\libclang_rt.builtins.a -lc -lm -o C:\buildbot\as-builder-1\x-armv7l\build\runtimes\runtimes-armv7-unknown-linux-gnueabihf-bins\compiler-rt\test\builtins\Unit\ARMHFLinuxConfig\Output\truncdfhf2_test.c.tmp && "C:/Python310/python.exe" "C:/buildbot/as-builder-1/x-armv7l/llvm-project/llvm/utils/remote-exec.py" --host=ubuntu@jetson6.lab.llvm.org C:\buildbot\as-builder-1\x-armv7l\build\runtimes\runtimes-armv7-unknown-linux-gnueabihf-bins\compiler-rt\test\builtins\Unit\ARMHFLinuxConfig\Output\truncdfhf2_test.c.tmp
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "C:/buildbot/as-builder-1/x-armv7l/build/./bin/clang.exe" "-gline-tables-only" "-gcodeview" "-gcolumn-info" "-Wthread-safety" "-Wthread-safety-reference" "-Wthread-safety-beta" "-fomit-frame-pointer" "-DCOMPILER_RT_ARMHF_TARGET" "-DCOMPILER_RT_HAS_FLOAT16" "-fno-builtin" "-I" "C:/buildbot/as-builder-1/x-armv7l/llvm-project/compiler-rt\lib\builtins" "-nodefaultlibs" "C:\buildbot\as-builder-1\x-armv7l\llvm-project\compiler-rt\test\builtins\Unit\truncdfhf2_test.c" "C:/buildbot/as-builder-1/x-armv7l/build/./lib/clang/16/lib/armv7-unknown-linux-gnueabihf\libclang_rt.builtins.a" "-lc" "-lm" "-o" "C:\buildbot\as-builder-1\x-armv7l\build\runtimes\runtimes-armv7-unknown-linux-gnueabihf-bins\compiler-rt\test\builtins\Unit\ARMHFLinuxConfig\Output\truncdfhf2_test.c.tmp"
$ "C:/Python310/python.exe" "C:/buildbot/as-builder-1/x-armv7l/llvm-project/llvm/utils/remote-exec.py" "--host=ubuntu@jetson6.lab.llvm.org" "C:\buildbot\as-builder-1\x-armv7l\build\runtimes\runtimes-armv7-unknown-linux-gnueabihf-bins\compiler-rt\test\builtins\Unit\ARMHFLinuxConfig\Output\truncdfhf2_test.c.tmp"
# command output:
error in test__truncdfhf2(nan) = 0000, expected 0x7e00
error: command failed with exit status: 1
--
********************

Unfortunately I cannot reproduce them. I tried with a Ubuntu 20.04 on aarch64 VM, but it succeeds with all tests. I have no idea what is different in the buildbot configuration.

If anybody has any clue about this, please let me know. Otherwise I'll probably put yet another kludge on top of the existing kludge, to avoid changing behavior for any platform other than FreeBSD.

compiler-rt/lib/builtins/CMakeLists.txt
804

Should move the removal of GENERIC_BF_SOURCES in this loop like this (Just wild guess. I'm neither familiar with compiler-rt nor cmake)