This is an archive of the discontinued LLVM Phabricator instance.

[builtins] Only build float16/bfloat16 code if actually supported
ClosedPublic

Authored by arichardson on Mar 3 2023, 5:54 AM.

Details

Summary

When building compiler-rt builtins for x86_64 they library will by default
also be built for i386. We unconditionally add the Float16 compile flags
since the check for Float16 support will be done using x86_64 compiler
flags, but i386 does not actually support it. Fix this by moving the
COMPILER_RT_HAS_FLOAT16 check to a per-target-architecture check.

Many of the checks in the builtin-config-ix file should probably also be
changed to per-target-arch checks, but so far only the Float16 one has
caused issues. This is an alternative to D136044 which added a special case
for i386 FreeBSD.

Fixes: https://github.com/llvm/llvm-project/issues/57224

Diff Detail

Event Timeline

arichardson created this revision.Mar 3 2023, 5:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2023, 5:54 AM
arichardson requested review of this revision.Mar 3 2023, 5:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2023, 5:54 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
arichardson added inline comments.Mar 3 2023, 6:00 AM
compiler-rt/lib/builtins/CMakeLists.txt
787
dim added a comment.Mar 3 2023, 6:39 AM

Note that the original problem was not only about _Float16 (COMPILER_RT_HAS_FLOAT16 in CMake), but also __bf16 (COMPILER_RT_HAS_BFLOAT16 in CMake). Both failed on i386-freebsd.

Thanks for the fix, but I may not be qualified to approve.

When building compiler-rt builtins for x86_64 they library will by default

they -> the

Also handle bfloat16 (didn't notice this before since I was building with clang 14)

arichardson added inline comments.Mar 6 2023, 3:57 AM
compiler-rt/lib/builtins/CMakeLists.txt
815

@pengfei Is there a reason bfloat16 is disabled for apple platforms, or was it the same x86_64/i386 problem that this patch fixes?

pengfei added inline comments.Mar 6 2023, 6:57 AM
compiler-rt/lib/builtins/CMakeLists.txt
815

I guess it is used to solve a build bot fail that cross compiling armv7 over x86_64, see comments on that patch. It might be the same reason as i386, but I didn't investigate it.

dim added inline comments.Mar 6 2023, 7:06 AM
compiler-rt/lib/builtins/CMakeLists.txt
815

Yes, that looks like D131147#3711517, where it probably detected bf16 support during the configure phase, when it was targeting aarch64. But then it used the same flags to compile armv7, where bf16 doesn't work. This is very similar to the x86_64/i386 situation, where x86_64 does support bf16, but i386 does not.

I _hope_ that arichardson's fix will make this work in a more correct fashion, i.e. do the detection again for each separate target. That would the issue for both aarch64/armv7 and x86_64/i386. (And maybe there are other such combinations...)

Fix bfloat16 case

arichardson retitled this revision from [builtins] Only build float16 code if actually supported to [builtins] Only build float16/bfloat16 code if actually supported.Mar 6 2023, 10:49 AM
arichardson edited the summary of this revision. (Show Details)
arichardson marked 2 inline comments as done.
arichardson edited the summary of this revision. (Show Details)

Drop APPLE special case

MaskRay added a subscriber: jrtc27.Mar 6 2023, 1:37 PM
arichardson edited the summary of this revision. (Show Details)Mar 7 2023, 12:40 AM
dim accepted this revision.Mar 7 2023, 11:19 AM

Yes, this looks good to me. Tried with very recent main:

...
-- Performing Test COMPILER_RT_HAS_ASM_LSE
-- Performing Test COMPILER_RT_HAS_ASM_LSE - Failed
-- Looking for __i386__
-- Looking for __i386__ - found
-- Builtin supported architectures: i386;x86_64
-- Performing additional configure checks with target flags: -m32
-- Performing Test COMPILER_RT_HAS_i386_FLOAT16
-- Performing Test COMPILER_RT_HAS_i386_FLOAT16 - Failed
-- Performing Test COMPILER_RT_HAS_i386_BFLOAT16
-- Performing Test COMPILER_RT_HAS_i386_BFLOAT16 - Failed
-- For i386 builtins preferring i386/fp_mode.c to fp_mode.c
-- For i386 builtins preferring i386/ashldi3.S to ashldi3.c
...
-- Performing additional configure checks with target flags: -m64
-- Performing Test COMPILER_RT_HAS_x86_64_FLOAT16
-- Performing Test COMPILER_RT_HAS_x86_64_FLOAT16 - Success
-- Performing Test COMPILER_RT_HAS_x86_64_BFLOAT16
-- Performing Test COMPILER_RT_HAS_x86_64_BFLOAT16 - Success
-- For x86_64 builtins preferring i386/fp_mode.c to fp_mode.c
-- For x86_64 builtins preferring x86_64/floatdidf.c to floatdidf.c
...

So indeed it detects i386 (with -m32) and x86_64 (with -m64) separately, and then both runtimes are built successfully:

[164/304] Linking C static library /home/dim/obj/llvmorg-17-init-4107-g439eebab81c3-freebsd14-amd64-ninja-clang-rel-1/lib/clang/17/lib/i386-unknown-freebsd14.0/libclang_rt.builtins.a
...
[304/304] Linking C static library /home/dim/obj/llvmorg-17-init-4107-g439eebab81c3-freebsd14-amd64-ninja-clang-rel-1/lib/clang/17/lib/x86_64-unknown-freebsd14.0/libclang_rt.builtins.a
This revision is now accepted and ready to land.Mar 7 2023, 11:19 AM
jrtc27 added inline comments.Mar 7 2023, 11:24 AM
compiler-rt/cmake/builtin-config-ix.cmake
15

I could believe this one could be a problem for some architectures. The others are probably fine... but I do wonder whether all of this should be made target-specific just to be safe. They're not causing immediate known issues though I guess.

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

Commit message should explain this change (not that it's hard to figure out)

813

Wonky indentation

arichardson added inline comments.Mar 7 2023, 12:53 PM
compiler-rt/cmake/builtin-config-ix.cmake
15

IMO we should just make all checks target dependent but in this patch I wanted to focus on the currently known broken cases before refactoring all the checks.

Will fix the outstanding issues and commit tomorrow. Thanks for reviewing and testing.

This revision was automatically updated to reflect the committed changes.
arichardson marked 2 inline comments as done.
chapuni added inline comments.
compiler-rt/lib/builtins/CMakeLists.txt
809

CMake's check_c_source_compiles() would fail if main() is not provided. Is it intentional?

In runtime builds, CMAKE_C_COMPILER_WORKS=ON hides the problem, though.

dim added inline comments.Jul 21 2023, 12:28 AM
compiler-rt/lib/builtins/CMakeLists.txt
809

You may be right, it looks as if the assumption is (not only in this case) that check_c_source_compiles() just checks if it compiles, not if it links. There are a few hand-made versions that do exactly that, but CMake's documentation states:

Check that the source supplied in <code> can be compiled as a C source file and linked as an executable (so it must contain at least a main() function).

So I think quite a lot of these instances might have to be replaced. Why didn't anybody notice this before? I certainly did not, and I think the function name check_c_source_compiles is very misleading...