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

[Anatoly Trosinenko via Phabricator via llvm-commits]

atrosinenko added a comment.

In D86453#2241702 <https://reviews.llvm.org/D86453#2241702>, @zatrazz wrote:

> 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.



================
Comment at: compiler-rt/test/builtins/Unit/extendhfsf2_test.c:10
 
-int test__extendhfsf2(uint16_t a, float expected)
+int test__extendhfsf2(TYPE_FP16 a, float expected)
 {
----------------
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.


================
Comment at: compiler-rt/test/builtins/Unit/extendhfsf2_test.c:17
         printf("error in test__extendhfsf2(%#.4x) = %f, "
-               "expected %f\n", a, x, expected);
+               "expected %f\n", fromRep16(a), x, expected);
     }
----------------
`toRep16(a)` is probably expected.


================
Comment at: compiler-rt/test/builtins/Unit/fp_test.h:6-10
+#ifdef COMPILER_RT_HAS_FLOAT16
+#define TYPE_FP16   _Float16
+#else
+#define TYPE_FP16   uint16_t
+#endif
----------------
zatrazz wrote:
> atrosinenko wrote:
> > This could probably be moved to fp_lib.h (or to int_types.h - looks like "int" means "internal" there...), possibly switching to `typedef`s.
> > 
> > This should make other code more idiomatic:
> > * there would be single-line `typedef`s 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:
> > 
> > ```lang=cpp
> > 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.
> 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.
Agree, looks like there is no more suitable place for that define right now.


================
Comment at: compiler-rt/test/builtins/Unit/fp_test.h:18
 {
+#ifdef COMPILER_RT_HAS_FLOAT16
+    TYPE_FP16 ret;
----------------
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...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86453/new/

https://reviews.llvm.org/D86453