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

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz added inline comments.


================
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)
 {
----------------
atrosinenko wrote:
> 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.
Using `uint32_t` indeed seems a better approach and its aligns somewhat to the change to use _Float16 where applicable. I will send a updated version with this fix.


================
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);
     }
----------------
atrosinenko wrote:
> `toRep16(a)` is probably expected.
Ack.


================
Comment at: compiler-rt/test/builtins/Unit/fp_test.h:18
 {
+#ifdef COMPILER_RT_HAS_FLOAT16
+    TYPE_FP16 ret;
----------------
atrosinenko wrote:
> 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...
I don't have a strong preference either, it should be optimized away by the compiler for `!COMPILER_RT_HAS_FLOAT16` anyway. I will keep to make it explicit that for `COMPILER_RT_HAS_FLOAT16` `TYPE_FP16` is expected to be different tha `uint16_t`.


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

https://reviews.llvm.org/D86453