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

[Luke Drummond via Phabricator via llvm-commits]

ldrumm marked an inline comment as done.
ldrumm added inline comments.


================
Comment at: compiler-rt/lib/builtins/extendhfdf2.c:19
+
+COMPILER_RT_ABI double __gnu_h2d_ieee(uint16_t a) { return __extendhfdf2(a); }
+
----------------
atrosinenko wrote:
> Where does the name `__gnu_h2d_ieee` comes from? Google finds this page only :)
Just copying the name mangling name used by libgcc. I can check with the gcc developers whether this is correct (libgcc doesn't have this conversion yet)




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


================
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);
----------------
atrosinenko wrote:
> 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.
I really don't see the value of that level of uniformity among programs that compute different things. This loop is incredibly simple and the extra abstraction doesn't really improve anything; a simple comparison and print statement has been moved somewhere else.

> Premature abstraction is the root of all grepping
> — Me


================
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;
----------------
atrosinenko wrote:
> 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.
The semantics being different here actually looks useful. I can see that in `compareResultD` the NaN checks distinguish signalling and quiet NaNs. I think that's what I wanted when I asked about NaNs in the original review request. Thanks. I'll see how it fares then update here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84877