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

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz added inline comments.


================
Comment at: compiler-rt/lib/builtins/extendhftf2.c:13
+
+#if defined(CRT_HAS_128BIT) && defined(CRT_LDBL_128BIT) && \
+    defined(COMPILER_RT_HAS_FLOAT16)
----------------
atrosinenko wrote:
> 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.
Right, I will fix it.


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


================
Comment at: compiler-rt/test/builtins/Unit/fp_test.h:85
 
-static inline int compareResultH(uint16_t result,
-                                 uint16_t expected)
+static inline int compareResultH(TYPE_FP16 result,
+                                 TYPE_FP16 expected)
----------------
atrosinenko wrote:
> zatrazz wrote:
> > atrosinenko wrote:
> > > 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.
> > 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.
> 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.
I think it should be feabile, it would require to redefine all the inputs in the test below though.


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

https://reviews.llvm.org/D86453