This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [builtins] Support conversion between fp16 and fp128
ClosedPublic

Authored by zatrazz on Nov 18 2020, 11:35 AM.

Details

Summary

This patch adds both extendhftf2 and trunctfhf2 to support
conversion between half-precision and quad-precision floating-point
values. They are enabled iff the compiler supports _Float16.

Some notes on ARM plaforms: while fp16 is supported on all
architectures, _Float16 is supported only for 32-bit ARM, 64-bit ARM,
and SPIR (as indicated by clang/docs/LanguageExtensions.rst). Also,
fp16 is a storage format and promoted to 'float' for argument passing
and 64-bit ARM supports floating-point convert precision to half as
base armv8-a instruction.

It means that although extendhfsf2, truncdfhf2 __truncsfhf2 will be
built for 64-bit ARM, they will be never used in practice (compiler
won't emit libcall to them). This patch does not change the ABI for
32-bit ARM, it will continue to pass _Float16 as uint16.

Diff Detail

Event Timeline

zatrazz created this revision.Nov 18 2020, 11:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2020, 11:35 AM
Herald added subscribers: Restricted Project, kristof.beyls, mgorny, dberris. · View Herald Transcript
zatrazz requested review of this revision.Nov 18 2020, 11:35 AM
ostannard accepted this revision.Nov 19 2020, 1:43 AM

LGTM

Also, __fp16 is a storage format and promoted to 'float' for argument passing

It doesn't affect this patch, but for all of the ARM and AArch64 ABIs, __fp16 arguments are passed in the bottom half of an integer or FP register, not promoted to float. There was an old version of the AArch32 ABI (I think) which did that, but I don't think any compiler implemented it before it was changed.

This revision is now accepted and ready to land.Nov 19 2020, 1:43 AM

LGTM

Also, __fp16 is a storage format and promoted to 'float' for argument passing

It doesn't affect this patch, but for all of the ARM and AArch64 ABIs, __fp16 arguments are passed in the bottom half of an integer or FP register, not promoted to float. There was an old version of the AArch32 ABI (I think) which did that, but I don't think any compiler implemented it before it was changed.

Right, I think I read on an older ACLE about it, but it does seem the newer version change it.

This revision was landed with ongoing or failed builds.Nov 19 2020, 10:15 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Nov 25 2020, 5:47 AM

This seems to break compiler-rt tests on darwin:

Testing:  0.. 10.. 
 FAIL: Builtins-x86_64-darwin :: divdf3_test.c (43 of 208)
 ******************** TEST 'Builtins-x86_64-darwin :: divdf3_test.c' FAILED ********************
 Script:
 --
 : 'RUN: at line 1';       /opt/s/w/ir/cache/builder/src/third_party/llvm-bootstrap/./bin/clang   -gline-tables-only  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.10 -isysroot /opt/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -DCOMPILER_RT_HAS_FLOAT16  -fno-builtin -I /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/builtins -nodefaultlibs /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/builtins/Unit/divdf3_test.c /opt/s/w/ir/cache/builder/src/third_party/llvm-bootstrap/./lib/clang/12.0.0/lib/darwin/libclang_rt.osx.a  -lSystem -o /opt/s/w/ir/cache/builder/src/third_party/llvm-bootstrap/projects/compiler-rt/test/builtins/Unit/X86_64DarwinConfig/Output/divdf3_test.c.tmp &&  /opt/s/w/ir/cache/builder/src/third_party/llvm-bootstrap/projects/compiler-rt/test/builtins/Unit/X86_64DarwinConfig/Output/divdf3_test.c.tmp
 --
 Exit Code: 1
 
 Command Output (stderr):
 --
 In file included from /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/builtins/Unit/divdf3_test.c:7:
 /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/builtins/Unit/fp_test.h:16:15: error: _Float16 is not supported on this target
 static inline TYPE_FP16 fromRep16(uint16_t x)
               ^
 /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/builtins/Unit/fp_test.h:7:19: note: expanded from macro 'TYPE_FP16'
 #define TYPE_FP16 _Float16
                   ^
[...]
 Failed Tests (6):
   Builtins-x86_64-darwin :: divdf3_test.c
   Builtins-x86_64-darwin :: divsf3_test.c
   Builtins-x86_64-darwin :: extendhfsf2_test.c
   Builtins-x86_64-darwin :: truncdfhf2_test.c
   Builtins-x86_64-darwin :: truncdfsf2_test.c
   Builtins-x86_64-darwin :: truncsfhf2_test.c
 
 
 Testing Time: 11.16s
   Unsupported      :  62
   Passed           : 139
   Expectedly Failed:   1
   Failed           :   6

Full log https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8863130845589077872/+/steps/package_clang/0/stdout?format=raw

Upstream bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1152762

Please take a look, and revert for now if it takes a while to fix.

rnk added a subscriber: rnk.Nov 25 2020, 4:13 PM

FYI, I went ahead and reverted this along with its dependent change since we didn't hear back.