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

Mon Aug 24 13:47:14 PDT 2020

zatrazz added inline comments.

================
Comment at: compiler-rt/lib/builtins/extendhftf2.c:1
+//===-- lib/extendhfsf2.c - half -> quad conversion ---------------*- C -*-===//
+//
----------------
atrosinenko wrote:
> `extendhfsf2.c` should probably be `extendhftf2.c`.
Indeed, I will update it.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86453