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

[Anatoly Trosinenko via Phabricator via llvm-commits]

atrosinenko added a reviewer: atrosinenko.
atrosinenko added a comment.

Thanks @ldrumm! Some comments on the test follow.



================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:19
+    uint64_t d;
+  } testcases[] = {
+#include "extendhfdf2_testcases.inc"
----------------
If I get it right, this `testcases` array will most probably be placed on the stack. As it is quite large, I would rather declare this `struct` separately and place it outside of `main()`:

```lang=cpp
struct testcase {
  uint16_t src16;
  uint64_t dst64;
};

static const struct testcase testcases[] = {
// ...
};

int main() {
  // ...
```


================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:28
+    } ret, expected;
+    expected.i = testcases[i].d;
+
----------------
If I remember correctly, such "write to one field, read from the another" cases may need special attention to not introduce Undefined Behavior. I would rather simply use `toRep64`, `fromRep64` and so on from `fp_test.h`.


================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:43
+  }
+  exit(err);
+}
----------------
I would rather just use `return err;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84877