Page MenuHomePhabricator

[AArch64] Support conversion between fp16 and fp128
Needs ReviewPublic

Authored by bryanpkc on Aug 13 2018, 8:58 PM.

Details

Summary

Add extendhftf2 and trunctfhf2, to support conversion between half-precision
and quad-precision floating-point values. On AArch64, use __fp16 as the half-
precision type so that such values are passed and returned in native FP16
registers. This is consistent with the behavior of the corresponding functions
in libgcc. Also fix some bugs in existing test cases.

Diff Detail

Event Timeline

bryanpkc created this revision.Aug 13 2018, 8:58 PM

Pinging reviewers...

This looks good to me, but it's not really my area of expertise, so would appreciate if someone else can have a look too. Perhaps @olista01 or @efriedma ?

efriedma added inline comments.Aug 28 2018, 12:08 PM
compiler-rt/lib/builtins/fp_extend.h
43

Can we implement this some other way? _Float16 should have the right calling convention on all targets, I think.

SjoerdMeijer added inline comments.Aug 29 2018, 12:15 AM
compiler-rt/lib/builtins/fp_extend.h
43

That was exactly the first thing I was thinking of. But then I also got confused about e.g. changing this:

float __aeabi_h2f(uint16_t a)

to:

float __aeabi_h2f(src a)

where src can now be __fp16. First I was wondering if changing the signature would be problematic somehow. Probably not, and probably it is more correct. Looking at the RT ABI for h2f, the descriptions reads:

IEEE 754 binary16 storage format (VFP half precision) to binary32 (float)  conversion

From a first glance, I could image that __fp16 would capture this behaviour, but I would need to check again what the binary16 storage format exactly is, and that's why I wasn't sure about all this.

bryanpkc added inline comments.Sep 3 2018, 11:37 PM
compiler-rt/lib/builtins/fp_extend.h
43

Thanks for your comments. Many of these FP16 conversion helpers should be unnecessary on AArch64 (when __ARM_FP16_ARGS is defined) since the backend can generate instructions for such conversions. However the implementations of the helpers are intended to be shared by all targets, so that is why I decided to limit the impact of my change with the conditional compilation directive above. Similarly I also concluded that changing the signature of __aeabi_h2f etc. would not be problematic for AArch64 with FP16 support, but to be conservative, we could change src_t back to uint16_t in the standard runtime helpers, and add an explicit cast of a from uint16_t to src_t for the call to __extendhfsf2 etc.

efriedma added inline comments.Sep 4 2018, 11:00 AM
compiler-rt/lib/builtins/fp_extend.h
43

Yes, the ifdefs limit the impact for now, but I would rather fix the issue everywhere once. Otherwise, we'll be forced to hack up fp_extend.h every time clang adds support for a new target where the calling convention for _Float16 is different from the calling convention for uint16_t.

shawnl added a subscriber: shawnl.Mon, Oct 14, 1:05 PM

The Zig language doesn't really want to have to special-case this conversion, and would like to see this patch be revisited. It seems to me like some of these reviews were almost accepts. https://github.com/ziglang/zig/issues/3282

Herald added a project: Restricted Project. · View Herald TranscriptMon, Oct 14, 1:05 PM

It looks like this is still waiting for review comments to be addressed.