This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [builtins] Detect _Float16 support at compile time
AbandonedPublic

Authored by dim on Jul 28 2022, 10:58 AM.

Details

Summary

Instead of detecting _Float16 support at CMake configuration time,
detect it at compile time by checking for the predefined (by the
compiler) macro __FLT16_MAX__ instead.

This solves the issue where compiler-rt is built simultaneously for both
x86_64 and i386 targets, and the CMake configuration uses x86_64
compilation to detect _Float16 support, while it may not be supported
by the i386 target (if it does not have SSE2).

While here, rename COMPILERT_RT_HAS_FLOAT16 to CRT_HAS_FLOAT16, to
conform more to the naming style used in int_lib.h and int_types.h.

Diff Detail

Event Timeline

dim created this revision.Jul 28 2022, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2022, 10:58 AM
dim requested review of this revision.Jul 28 2022, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2022, 10:58 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
dim updated this revision to Diff 448387.Jul 28 2022, 10:59 AM

Fix typo.

Is this something we need in the release branch?

dim added a comment.Jul 28 2022, 1:18 PM

Is this something we need in the release branch?

Yes please, provided that it's acceptable to everybody of course. I ran into this while building release/15.x after I had updated my host compiler to a very recent main snapshot (llvmorg-15-init-17485-ga3e38b4a206b). E.g. building release/15.x or recent main with clang 14.0.5 will work just fine, because it didn't yet support _Float16. But after rG655ba9c8a1d22075443711cc749f0b032e07adee landed, of course it did start supporting _Float16.

@kparzysz reminded me. This is not the right direction to solve the problem. The ABI of runtime libraries should be constant rather than changing with different features. Imagining a program may be built with or without SSE2 and linked with the same runtime. Another problem is compatibility with libgcc. Using uint16_t is not compatible with libgcc anymore.

Fortunately, LLVM codegen (unfortunately Darwin isn't) was using __gnu_h2f_ieee/__gnu_f2h_ieee before the change https://godbolt.org/z/GnrT5vGr1. So an easy way to solve the problem is always assign -msse2 when build extendhftf2/trunctfhf2.

I have a patch D128872 for Darwin targets, I hope we can put it into the 15.x release.

Or we simply refuse to build them if COMPILER_RT_HAS_FLOAT16 is not set.

MaskRay accepted this revision.Jul 28 2022, 9:24 PM

LGTM. But I hope that others can check their builds.

This revision is now accepted and ready to land.Jul 28 2022, 9:24 PM
dim added a comment.Jul 29 2022, 11:57 AM

@kparzysz reminded me. This is not the right direction to solve the problem. The ABI of runtime libraries should be constant rather than changing with different features. Imagining a program may be built with or without SSE2 and linked with the same runtime. Another problem is compatibility with libgcc. Using uint16_t is not compatible with libgcc anymore.

Fortunately, LLVM codegen (unfortunately Darwin isn't) was using __gnu_h2f_ieee/__gnu_f2h_ieee before the change https://godbolt.org/z/GnrT5vGr1. So an easy way to solve the problem is always assign -msse2 when build extendhftf2/trunctfhf2.

I have a patch D128872 for Darwin targets, I hope we can put it into the 15.x release.

If this is the case, then maybe the whole commit that introduced the use of _Float16 (i.e. rG655ba9c8a1d22075443711cc749f0b032e07adee) should be reverted *again*? (It's already titled 'Reland "Reland "Reland "Reland "[X86][RFC] Enable _Float16 type support on X86 following the psABI""""', so relanding it One More Time shouldn't be that difficult ;))

That said, rG655ba9c8a1d22075443711cc749f0b032e07adee's description also says "following the psABI" so maybe this is actually going to improve ABI compatibility?

This revision was landed with ongoing or failed builds.Jul 29 2022, 11:59 AM
This revision was automatically updated to reflect the committed changes.
fmayer added a subscriber: fmayer.Jul 29 2022, 5:23 PM

I think this broke the sanitizer-windows buildbot: https://lab.llvm.org/buildbot/#/builders/127/builds/33583/steps/4/logs/stdio

[99/120] Running the Builtins tests
-- Testing: 214 tests, 16 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60..
FAIL: Builtins-x86_64-windows :: truncdfhf2_test.c (53 of 214)
******************** TEST 'Builtins-x86_64-windows :: truncdfhf2_test.c' FAILED ********************
Script:
--
: 'RUN: at line 1';       C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe   -gline-tables-only -gcodeview -gcolumn-info     -fno-builtin -I C:/b/slave/sanitizer-windows/llvm-project/compiler-rt\lib\builtins -nodefaultlibs C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\builtins\Unit\truncdfhf2_test.c C:/b/slave/sanitizer-windows/build/stage1/./lib/clang/16.0.0/lib/windows\clang_rt.builtins-x86_64.lib -o C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncdfhf2_test.c.tmp &&  C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncdfhf2_test.c.tmp
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe" "-gline-tables-only" "-gcodeview" "-gcolumn-info" "-fno-builtin" "-I" "C:/b/slave/sanitizer-windows/llvm-project/compiler-rt\lib\builtins" "-nodefaultlibs" "C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\builtins\Unit\truncdfhf2_test.c" "C:/b/slave/sanitizer-windows/build/stage1/./lib/clang/16.0.0/lib/windows\clang_rt.builtins-x86_64.lib" "-o" "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncdfhf2_test.c.tmp"
$ "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncdfhf2_test.c.tmp"
# command output:
error in test__truncdfhf2(nan) = 0000, expected 0x7e00
error: command failed with exit status: 1
--
********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.
FAIL: Builtins-x86_64-windows :: truncsfhf2_test.c (212 of 214)
******************** TEST 'Builtins-x86_64-windows :: truncsfhf2_test.c' FAILED ********************
Script:
--
: 'RUN: at line 1';       C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe   -gline-tables-only -gcodeview -gcolumn-info     -fno-builtin -I C:/b/slave/sanitizer-windows/llvm-project/compiler-rt\lib\builtins -nodefaultlibs C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\builtins\Unit\truncsfhf2_test.c C:/b/slave/sanitizer-windows/build/stage1/./lib/clang/16.0.0/lib/windows\clang_rt.builtins-x86_64.lib -o C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncsfhf2_test.c.tmp &&  C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncsfhf2_test.c.tmp
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe" "-gline-tables-only" "-gcodeview" "-gcolumn-info" "-fno-builtin" "-I" "C:/b/slave/sanitizer-windows/llvm-project/compiler-rt\lib\builtins" "-nodefaultlibs" "C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\builtins\Unit\truncsfhf2_test.c" "C:/b/slave/sanitizer-windows/build/stage1/./lib/clang/16.0.0/lib/windows\clang_rt.builtins-x86_64.lib" "-o" "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncsfhf2_test.c.tmp"
$ "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncsfhf2_test.c.tmp"
# command output:
error in test__truncsfhf2(nan) = 0000, expected 0x7e00
error: command failed with exit status: 1
--

@kparzysz reminded me. This is not the right direction to solve the problem. The ABI of runtime libraries should be constant rather than changing with different features. Imagining a program may be built with or without SSE2 and linked with the same runtime. Another problem is compatibility with libgcc. Using uint16_t is not compatible with libgcc anymore.

Fortunately, LLVM codegen (unfortunately Darwin isn't) was using __gnu_h2f_ieee/__gnu_f2h_ieee before the change https://godbolt.org/z/GnrT5vGr1. So an easy way to solve the problem is always assign -msse2 when build extendhftf2/trunctfhf2.

I have a patch D128872 for Darwin targets, I hope we can put it into the 15.x release.

If this is the case, then maybe the whole commit that introduced the use of _Float16 (i.e. rG655ba9c8a1d22075443711cc749f0b032e07adee) should be reverted *again*? (It's already titled 'Reland "Reland "Reland "Reland "[X86][RFC] Enable _Float16 type support on X86 following the psABI""""', so relanding it One More Time shouldn't be that difficult ;))

It is nonsense to take the revert times as a reason.

That said, rG655ba9c8a1d22075443711cc749f0b032e07adee's description also says "following the psABI" so maybe this is actually going to improve ABI compatibility?

The psABI refers in particular to https://gitlab.com/x86-psABIs. There're some instruction limitations to make us only support _Float16 for SSE2 and up, see the discussion https://groups.google.com/g/llvm-dev/c/k5WLPVGSeWU. And if you disable SSE2, you will get error on GCC too https://godbolt.org/z/33v7r9noP.

That said, build and provide the FP16 runtime without SSE2 is out of the scope of psABI. As for a general ABI compatibility, e.g., keeping the same calling conversion as pre rG655ba9c in cases without SSE2, this patch also doesn't help much, because for them we use __gnu_h2f_ieee/__gnu_f2h_ieee rather than extendhftf2/trunctfhf2 (except for Darwin).

pengfei reopened this revision.Jul 29 2022, 6:43 PM

I think this broke the sanitizer-windows buildbot: https://lab.llvm.org/buildbot/#/builders/127/builds/33583/steps/4/logs/stdio

[99/120] Running the Builtins tests
-- Testing: 214 tests, 16 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60..
FAIL: Builtins-x86_64-windows :: truncdfhf2_test.c (53 of 214)
******************** TEST 'Builtins-x86_64-windows :: truncdfhf2_test.c' FAILED ********************
Script:
--
: 'RUN: at line 1';       C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe   -gline-tables-only -gcodeview -gcolumn-info     -fno-builtin -I C:/b/slave/sanitizer-windows/llvm-project/compiler-rt\lib\builtins -nodefaultlibs C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\builtins\Unit\truncdfhf2_test.c C:/b/slave/sanitizer-windows/build/stage1/./lib/clang/16.0.0/lib/windows\clang_rt.builtins-x86_64.lib -o C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncdfhf2_test.c.tmp &&  C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncdfhf2_test.c.tmp
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe" "-gline-tables-only" "-gcodeview" "-gcolumn-info" "-fno-builtin" "-I" "C:/b/slave/sanitizer-windows/llvm-project/compiler-rt\lib\builtins" "-nodefaultlibs" "C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\builtins\Unit\truncdfhf2_test.c" "C:/b/slave/sanitizer-windows/build/stage1/./lib/clang/16.0.0/lib/windows\clang_rt.builtins-x86_64.lib" "-o" "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncdfhf2_test.c.tmp"
$ "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncdfhf2_test.c.tmp"
# command output:
error in test__truncdfhf2(nan) = 0000, expected 0x7e00
error: command failed with exit status: 1
--
********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.
FAIL: Builtins-x86_64-windows :: truncsfhf2_test.c (212 of 214)
******************** TEST 'Builtins-x86_64-windows :: truncsfhf2_test.c' FAILED ********************
Script:
--
: 'RUN: at line 1';       C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe   -gline-tables-only -gcodeview -gcolumn-info     -fno-builtin -I C:/b/slave/sanitizer-windows/llvm-project/compiler-rt\lib\builtins -nodefaultlibs C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\builtins\Unit\truncsfhf2_test.c C:/b/slave/sanitizer-windows/build/stage1/./lib/clang/16.0.0/lib/windows\clang_rt.builtins-x86_64.lib -o C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncsfhf2_test.c.tmp &&  C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncsfhf2_test.c.tmp
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe" "-gline-tables-only" "-gcodeview" "-gcolumn-info" "-fno-builtin" "-I" "C:/b/slave/sanitizer-windows/llvm-project/compiler-rt\lib\builtins" "-nodefaultlibs" "C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\builtins\Unit\truncsfhf2_test.c" "C:/b/slave/sanitizer-windows/build/stage1/./lib/clang/16.0.0/lib/windows\clang_rt.builtins-x86_64.lib" "-o" "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncsfhf2_test.c.tmp"
$ "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\builtins\Unit\X86_64WindowsConfig\Output\truncsfhf2_test.c.tmp"
# command output:
error in test__truncsfhf2(nan) = 0000, expected 0x7e00
error: command failed with exit status: 1
--

Thanks for the reporting. Reverted.

This revision is now accepted and ready to land.Jul 29 2022, 6:43 PM
dim added a comment.Jul 30 2022, 1:55 AM

Okay, so do we continue with this approach at all, then? The problem that x86_64 builds fail by default on non-gcc compilers still stands. I'm just not good enough in handling the CMake logic to figure that out, if we have to handle it via CMake.

Neither do I. The compiler-rt files and the CMake is a maze to me. For example, we have truncsfhf2.c/extendhfsf2.c;trunctfhf2.c/extendhftf2.c;truncdfhf2.c in compiler-rt, but we are missing __extendhfdf2, __truncxfhf2/__extendhfxf2 compared to gcc. Besides, truncsfhf2.c/extendhfsf2.c/truncdfhf2.c use src_t/dst_t rather than _Float16 used in trunctfhf2.c/extendhftf2.c. I wonder if you can solve your problem by just replacing _Float16 by src_t/dst_t. However, it cannot explain why the buildbot failed only in truncsfhf2_test.c/truncdfhf2_test.c while passed in extendhfsf2_test.c ...

From the view point of a general solution, I think we can add -msse2 for X86 in the CMake. I think forcing it is not a problem given GCC has already implies it as you observed. OTOH, when you using configuration generated for x86_64 for i386 targets, you have acquiesced SSE2 is usable on i386. Otherwise, it is misuse in such scenario.

The problem that x86_64 builds fail by default on non-gcc compilers still stands.

It fails for GCC too. cc -m32 does not set -msse2 by default in vanilla GCC, see https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366/20?u=jwakely

pengfei requested changes to this revision.Sep 30 2022, 5:59 AM

The problem that x86_64 builds fail by default on non-gcc compilers still stands.

It fails for GCC too. cc -m32 does not set -msse2 by default in vanilla GCC, see https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366/20?u=jwakely

Thanks for the information! Then I believe the right way is to always pass -m32 -msse2 together when using the type.

This revision now requires changes to proceed.Sep 30 2022, 5:59 AM

What do we need to move forward with the patch?

HEAD as-is is broken on Ubuntu 22.10
https://lab.llvm.org/buildbot/#/builders/169/builds/13450/steps/7/logs/stdio

What do we need to move forward with the patch?

HEAD as-is is broken on Ubuntu 22.10
https://lab.llvm.org/buildbot/#/builders/169/builds/13450/steps/7/logs/stdio

I noticed the bot is starting to fail 3 days ago, but the patch merged for months. Is there any recent change exposed the problem?

As disscussed in https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366/25 the build of 32-bits using 64-bit config is the root cause. I think always turning on SSE2 explicitly in the 64-bit config should be a feasible workaround.

dim added a comment.Nov 9 2022, 2:58 AM

What do we need to move forward with the patch?

HEAD as-is is broken on Ubuntu 22.10
https://lab.llvm.org/buildbot/#/builders/169/builds/13450/steps/7/logs/stdio

I noticed the bot is starting to fail 3 days ago, but the patch merged for months. Is there any recent change exposed the problem?

As disscussed in https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366/25 the build of 32-bits using 64-bit config is the root cause. I think always turning on SSE2 explicitly in the 64-bit config should be a feasible workaround.

I have another patch coming up, which I have been using for the 15.0.x test builds.

What do we need to move forward with the patch?

HEAD as-is is broken on Ubuntu 22.10
https://lab.llvm.org/buildbot/#/builders/169/builds/13450/steps/7/logs/stdio

I noticed the bot is starting to fail 3 days ago, but the patch merged for months. Is there any recent change exposed the problem?

3 days was caused by unrelated reasons, which I fixed already.
yesterday I tried to update the bot from 22.04 ( with clang 14....) -> 22.10 ( with clang 15.0.4). I reverted it back to 22.04, but would like to upgrade it later again.

What do we need to move forward with the patch?

HEAD as-is is broken on Ubuntu 22.10
https://lab.llvm.org/buildbot/#/builders/169/builds/13450/steps/7/logs/stdio

I noticed the bot is starting to fail 3 days ago, but the patch merged for months. Is there any recent change exposed the problem?

3 days was caused by unrelated reasons, which I fixed already.
yesterday I tried to update the bot from 22.04 ( with clang 14....) -> 22.10 ( with clang 15.0.4). I reverted it back to 22.04, but would like to upgrade it later again.

I see, thanks for the information.

dim abandoned this revision.Jan 22 2023, 1:16 PM

Abandoning in favor of D136044.

luke957 removed a subscriber: luke957.Jan 23 2023, 1:30 AM