Page MenuHomePhabricator

[AArch64] Support conversion between fp16 and fp128
Needs ReviewPublic

Authored by zatrazz on Aug 24 2020, 6:07 AM.

Details

Summary

This is an updated version of https://reviews.llvm.org/D50685 .
The main changes are that

  • I have added a CMake test to check if compiler support _Float16 to avoid either to require a more recent gcc to build a stage1 compiler and avoid creating potentially invalid soft-fp symbols (as for ARM fp16 one when built with a compiler configured for hard-float).
  • Adjusted the internal tests to use _Float16 as well if compiler supports.

This issue has been brought by some developer from Apache TVM, where is
trigger an compiler ICE with the missing fp conversion.


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.

It also adjust the extendhfsf2, truncdfhf2 __truncsfhf2 to use
_Float16 when compiler supports it. On AArch64 it allows use the
native FP16 ABI, while on other architectures the expected current
semantic is preserved (arm for instance).

Diff Detail

Event Timeline

zatrazz created this revision.Aug 24 2020, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2020, 6:07 AM
Herald added subscribers: Restricted Project, danielkiss, hiraditya and 2 others. · View Herald Transcript
zatrazz requested review of this revision.Aug 24 2020, 6:07 AM
atrosinenko added inline comments.
compiler-rt/lib/builtins/extendhftf2.c
2

extendhfsf2.c should probably be extendhftf2.c.

compiler-rt/test/builtins/Unit/fp_test.h
86

Shouldn't it have (TYPE_FP16 result, uint16_t expected) arguments like other compareResultX functions (as if fp_t result, rep_t expected)? The same for test_truncXfhf2(... , uint16_t expected): if I get it right, the common tradition for them is expressing the reference value as corresponding uintXX_t. Meanwhile, this is probably related to your changes to second test case from extendhfsf2_test.c - the original sources look quite suspicious to me.

zatrazz added inline comments.Aug 24 2020, 1:47 PM
compiler-rt/lib/builtins/extendhftf2.c
2

Indeed, I will update it.

compiler-rt/test/builtins/Unit/fp_test.h
86

Yeah, I agree it should follow the other compare functions and use a uintxx_t as second argument, I will fix it. The extendhfsf2_test.c issue is in fact that it is using a wrong comparison function that is not making explicit what it is currently testing. The test__extendhfsf2 has a 'float' as second argument, however calls compareResultH which expectes a uint16_t (without this change). This makes the compiler to silent cast the result and the expected results could be misleading.

I think it should call compareResultF with toRep32 instead and to make explicit it compares against the uint32_t representation of the float, same as extenddftf2_test.c and extendsftf2_test.c are already doing. I will change it as well.

zatrazz updated this revision to Diff 287488.Aug 24 2020, 1:57 PM

Updated version based on previous comments.

atrosinenko added inline comments.Aug 25 2020, 3:28 AM
compiler-rt/lib/builtins/extendhftf2.c
14

Technically, linter is used to complain about \ being not at the 80th column for multi-line macroses, but I'm not sure all these rules apply to compiler-rt/builtins library.

compiler-rt/test/builtins/Unit/fp_test.h
6–10

This could probably be moved to fp_lib.h (or to int_types.h - looks like "int" means "internal" there...), possibly switching to typedefs.

This should make other code more idiomatic:

  • there would be single-line typedefs for src_t and dst_t in fp_extend.h and fp_trunc.h, just as for other precisions
  • the precision converting LibCalls (extend / trunc) would look more idiomatically:
COMPILER_RT_ABI concrete_return_type __libcallXfYf2(concrete_argument_type) { ... }

... with one of those types being TYPE_F16 alias instead of src_t / dst_t.

86

It would probably be even more idiomatically to declare test__extendhfsf2 with the second argument being uint32_t expected, but this may cause quite large changes that are not strictly related to this patch.

zatrazz added inline comments.Aug 25 2020, 6:47 AM
compiler-rt/lib/builtins/extendhftf2.c
14

Right, I will fix it.

compiler-rt/test/builtins/Unit/fp_test.h
6–10

The fp_lib.h now only defines a single floating-point type (one must define either SINGLE_PRECISION, DOUBLE_PRECISION, or QUAD_PRECISION). For instance on extendsftf2.c, it defines QUAD_PRECISION and includes fp_lib.h. To move the float16 definition to fp_lib.h it would require to add the possibility to define multiple types, similar to what is already done to fp_extend.h/fp_trunc.h. I am not sure if is better, the second fp type is used currently on the extend/trunc builtins.

And I don' t think int_types.h is the correct place, afaiks _Float16 currently not really defined on all architectures so its not really related to an interger type.

Ideally I think compiler-rt should define *all* 'hf' builtins to use _Float16 and built them iff the ABI implements the type (meaning the compiler actually emits libcalls to it). For ABI that support float16 operations without supporting _Float16 type, for instance ARM which supports __fp16, it would be better to move the libcall implementation to the arch-specific folders.

86

I think it should be feabile, it would require to redefine all the inputs in the test below though.

ldrumm added a subscriber: ldrumm.Aug 25 2020, 7:17 AM

Hi atrosinenko, do you think this patch need any more change on the testing side?
The fp_lib.h/int_lib.h change would most likely require in a more complex without
much gain in organization imho.

Hi atrosinenko, do you think this patch need any more change on the testing side?
The fp_lib.h/int_lib.h change would most likely require in a more complex without
much gain in organization imho.

Sorry for the delay. Provided the particular testcases are correct, I have found just a single fromRep16 that was probably used instead of toRep16. Other comments are merely random thoughts just in case.

compiler-rt/test/builtins/Unit/extendhfsf2_test.c
10

As previously discussed, I would rather use uint32_t expected instead. But after all, the objective of this patch is not to fix test__extendhfsf2, so even if this would be implemented, then it should probably go to a separate patch and not clutter this one. Commenting this just for completeness.

17

toRep16(a) is probably expected.

compiler-rt/test/builtins/Unit/fp_test.h
6–10

Agree, looks like there is no more suitable place for that define right now.

18

If I get it right, this could be performed unconditionally. On the other hand, the #else branch may be either a good illustration for peculiarities of this function when no native _Float16 is available or some misleading stuff...

zatrazz added inline comments.Aug 31 2020, 10:42 AM
compiler-rt/test/builtins/Unit/extendhfsf2_test.c
10

Using uint32_t indeed seems a better approach and its aligns somewhat to the change to use _Float16 where applicable. I will send a updated version with this fix.

17

Ack.

compiler-rt/test/builtins/Unit/fp_test.h
18

I don't have a strong preference either, it should be optimized away by the compiler for !COMPILER_RT_HAS_FLOAT16 anyway. I will keep to make it explicit that for COMPILER_RT_HAS_FLOAT16 TYPE_FP16 is expected to be different tha uint16_t.

zatrazz updated this revision to Diff 288990.Aug 31 2020, 10:46 AM

Updated patch based on previous comments.

MaskRay added inline comments.
compiler-rt/lib/builtins/fp_extend.h
43

#ifdef ? ditto below

compiler-rt/test/builtins/Unit/trunctfhf2_test.c
16

Use 2-space indentation. Place { in the end. There is no need following the style of some violating files in this directory.

zatrazz updated this revision to Diff 289221.Sep 1 2020, 10:43 AM

I have adapted the news files using clang-format and fixed minor style issues pointed by previous comments.

MaskRay added inline comments.Sep 8 2020, 10:48 AM
compiler-rt/lib/builtins/fp_extend.h
43

You can mark the comment above as "Done".

_Float16 and uint16_t are different types. Do all the *hf* functions change their signatures when _Float16 is supported? This still looks strange.

zatrazz marked an inline comment as done.Sep 8 2020, 11:51 AM
zatrazz added inline comments.
compiler-rt/lib/builtins/fp_extend.h
43

I think using _Float16 for generic support does make more sense, since float16 support is really an extension and with different calling convention on each architecture and even on some different ABIs. While it is uint16_t for armv6 due the soft-fp ABI, it is a complete different type with a different calling convention for aarch64. And I expect that other architectures to use similar strategies now that it is supported on some vector extensions in some new chips (POWER10 for instance).

Also, for ABI which expects *hf* to have 'uint16_t' as calling convetion it would be better to compartmentalize it to the specific arch folder, for instance for arm (and this is what libgcc does for instance).

MaskRay added a subscriber: ab.Sep 8 2020, 12:12 PM
MaskRay added inline comments.
compiler-rt/lib/builtins/fp_extend.h
43

If 'uint16_t' is more of an anomaly, @ab should the arm softfp implementation be moved to the specific arch folder?

zatrazz added inline comments.Sep 8 2020, 12:58 PM
compiler-rt/lib/builtins/fp_extend.h
43

I would say so, although I am not fully sure if any other architecture does use such routines (which in this case would need to add such routines as well).