Page MenuHomePhabricator

Enable `_Float16` type support on X86 without the avx512fp16 flag
AcceptedPublic

Authored by zahiraam on Nov 17 2021, 8:59 AM.

Details

Summary

The _Float16 type is supported on x86 systems with SSE2 enabled. Operations are emulated by software emulation and “float” instructions. This patch is allowing the support of _Float16 type without the use of -max512fp16 flag. The final goal being, perform _Float16 emulation for all arithmetic expressions.

Diff Detail

Event Timeline

zahiraam requested review of this revision.Nov 17 2021, 8:59 AM
zahiraam created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 8:59 AM
rjmccall added inline comments.Nov 17 2021, 4:03 PM
clang/lib/Basic/Targets/X86.cpp
242

We should probably be setting HasLegalHalfType here.

373

Out of curiosity, why SSE2? SSE2 adds double-precision, but we just need single-precision, which is in the original SSE.

It doesn't much matter anymore, of course, since both are ubiquitous.

pengfei added inline comments.Nov 17 2021, 6:13 PM
clang/lib/Basic/Targets/X86.cpp
372

sse2?

373
clang/test/CodeGen/X86/avx512fp16-abi.c
1

Why don't move it to fp16-abi.c directly?

clang/test/CodeGen/X86/avx512fp16-complex.c
1

Change the file name?

rjmccall added inline comments.Nov 17 2021, 6:23 PM
clang/lib/Basic/Targets/X86.cpp
373

Ah, makes sense.

zahiraam updated this revision to Diff 388179.Nov 18 2021, 6:23 AM
zahiraam marked 7 inline comments as done.
tschuett added inline comments.
clang/docs/LanguageExtensions.rst
676

Would something like SSE2 and up help understanding?

zahiraam updated this revision to Diff 388189.Nov 18 2021, 7:02 AM
zahiraam marked an inline comment as done.
rjmccall requested changes to this revision.Nov 18 2021, 8:37 AM
rjmccall added inline comments.
clang/test/SemaCXX/Float16.cpp
6

This test (and Float16.c) should test at least one target that doesn't have _Float16 support, so please just add -DHAVE to the x86_64 line and add, I dunno, a generic i386 or SPARC line.

This revision now requires changes to proceed.Nov 18 2021, 8:37 AM
zahiraam updated this revision to Diff 388273.Nov 18 2021, 11:20 AM
zahiraam marked an inline comment as done.
rjmccall accepted this revision.Nov 18 2021, 10:35 PM

Thanks, LGTM

This revision is now accepted and ready to land.Nov 18 2021, 10:35 PM

As mentioned in
https://github.com/llvm/llvm-project/commit/6623c02d70c3732dbea59c6d79c69501baf9627b#commitcomment-60741407

This change is breaking build of compiler-rt on Ubuntu bionic and others on amd64:

"/build/llvm-toolchain-snapshot-14~++20211119100719+d729f4c38fca/build-llvm/./bin/clang" --target=x86_64-pc-linux-gnu -DVISIBILITY_HIDDEN  -fstack-protector-strong -Wformat -Werror=format-security -Wno-unused-command-line-argument -Wdate-time -D_FORTIFY_SOURCE=2 -O3 -DNDEBUG  -m32 -DCOMPILER_RT_HAS_FLOAT16 -std=c11 -fPIC -fno-builtin -fvisibility=hidden -fomit-frame-pointer -MD -MT CMakeFiles/clang_rt.builtins-i386.dir/extendhfsf2.c.o -MF CMakeFiles/clang_rt.builtins-i386.dir/extendhfsf2.c.o.d -o CMakeFiles/clang_rt.builtins-i386.dir/extendhfsf2.c.o -c '/build/llvm-toolchain-snapshot-14~++20211119100719+d729f4c38fca/compiler-rt/lib/builtins/extendhfsf2.c'
In file included from /build/llvm-toolchain-snapshot-14~++20211119100719+d729f4c38fca/compiler-rt/lib/builtins/extendhfsf2.c:11:
In file included from /build/llvm-toolchain-snapshot-14~++20211119100719+d729f4c38fca/compiler-rt/lib/builtins/fp_extend_impl.inc:38:
/build/llvm-toolchain-snapshot-14~++20211119100719+d729f4c38fca/compiler-rt/lib/builtins/fp_extend.h:44:9: error: _Float16 is not supported on this target
typedef _Float16 src_t;
        ^
1 error generated.

Actually, it breaks on all Debian.
Could you please revert it?

Actually, it breaks on all Debian.
Could you please revert it?

Done.

Actually, it breaks on all Debian.
Could you please revert it?

Done.

I have reverted this patch but would like to push it in at some point (may be after the back end changes https://reviews.llvm.org/D107082 will be merged in.
But I see in the command above that it is compiling with -DCOMPILER_RT_HAS_FLOAT16. @sylvestre.ledru is this flag really supposed to be on? Was it the case before this patch? @rjmccall isn't this because we turned on HasLegalHalfType?