[PATCH] D84877: Support for soft fp16 to fp64 IEEE conversions

Anatoly Trosinenko via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 24 10:02:40 PDT 2020


atrosinenko added a comment.

Sorry for such an iterative feedback. A couple of general thoughts:

- It may be worth looking at D86453: [AArch64] Support conversion between fp16 and fp128 <https://reviews.llvm.org/D86453>: if I get it right, it introduces a `TYPE_FP16` macro to be used instead of `uint16_t` when using it instead of half-size float. When one of this patch and D86453 <https://reviews.llvm.org/D86453> lands, the other one will probably need some updates
- Unlike many other tests, this one always checks all the test cases instead of failing with non-zero exit code on the first failed one. If something goes wrong with this libcall, build log can be flooded with up to 65k warning lines. Still I'm not sure whether this is an issue.



================
Comment at: compiler-rt/lib/builtins/extendhfdf2.c:19
+
+COMPILER_RT_ABI double __gnu_h2d_ieee(uint16_t a) { return __extendhfdf2(a); }
+
----------------
Where does the name `__gnu_h2d_ieee` comes from? Google finds this page only :)


================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:15-16
+static const struct {
+  uint16_t i;
+  uint64_t d;
+} testcases[] = {
----------------
It may be worth giving these fields more descriptive names, something like `argument` and `expected_result` or so.


================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:26
+    double expected_f = fromRep64(expected_i);
+    double result_f = __extendhfdf2(testcases[i].i);
+    uint64_t result_i = toRep64(result_f);
----------------
In most of the tests here, an actual libcall invocation is usually performed inside a separate function declared like this
    int test__extendhfdf2(uint16_t a, uint64_t expected)
This function prints a message in case of an error, too.

I guess this is much less significant for a table-driven test. On the other hand, this seems to not harm here as well and makes the tests look a bit more uniformly.


================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:28-30
+    if (result_i != expected_i) {
+      if (isnan(result_f) && isnan(expected_f))
+        continue;
----------------
Using `compareResultD` could probably eliminate this hand-written comparison. It has a slightly different semantics for NaN, though. Some of extendXfYf2 tests use it (as well as tests for many other fp-related libcalls), so it should //probably// be usable (except for adjusting extendhfdf2_testcases.inc). `#include <math.h>` could probably be dropped then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84877



More information about the llvm-commits mailing list