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

[Luke Drummond via Phabricator via llvm-commits]

ldrumm added inline comments.


================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:19
+    uint64_t d;
+  } testcases[] = {
+#include "extendhfdf2_testcases.inc"
----------------
atrosinenko wrote:
> 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() {
>   // ...
> ```
Yep. Good point. Not everyone has an 8MiB stack by default. I'll make it a global


================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:28
+    } ret, expected;
+    expected.i = testcases[i].d;
+
----------------
atrosinenko wrote:
> 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`.
I worry that the rumours that type punning through a union being undefined have become greatly exaggerated. However I should perhaps save my rants for the C standards committee for not clearing up their mess. I accept your offer of helper functions ;)


================
Comment at: compiler-rt/test/builtins/Unit/extendhfdf2_test.c:34-35
+          continue;
+        printf(
+            "error in test__extendhfdf2(%#.4x) = %f, expected %f\n",
+            testcases[i].i,
----------------
indentation is all over the place


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84877