[PATCH] D50685: [AArch64] Support conversion between fp16 and fp128

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: compiler-rt/lib/builtins/fp_extend.h:43
 #elif defined SRC_HALF
+#if defined __aarch64__ && defined __ARM_FP16_ARGS
+// Use __fp16 if available.
----------------
bryanpkc wrote:
> SjoerdMeijer wrote:
> > efriedma wrote:
> > > Can we implement this some other way?  _Float16 should have the right calling convention on all targets, I think.
> > 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.
> > 
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D50685