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

[Anatoly Trosinenko via Phabricator via llvm-commits]

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


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


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


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

https://reviews.llvm.org/D86453