[PATCH] D63061: [builtins] Fix overflow issue for complex division with big numbers

Wed Nov 20 06:11:46 PST 2019

scanon requested changes to this revision.
scanon added inline comments.
This revision now requires changes to proceed.

================
Comment at: lib/builtins/divdc3.c:34
+  COMPLEX_REAL(z) = (__a * __c + __b * __d) / __denom;
+  COMPLEX_IMAGINARY(z) = (__b * __c - __a * __d) / __denom;
   if (crt_isnan(COMPLEX_REAL(z)) && crt_isnan(COMPLEX_IMAGINARY(z))) {
----------------
We don't have a clear definition of the required semantics for these builtins, but should FMA formation be allowed here?

As written, it looks like we're at the mercy of the fp-contract flags passed while building compiler-rt, which means that the exact cancellation assumed by the tests won't pass if the default is ever "on". I would suggest either relaxing the tests or adding an explicit `#pragma STDC FP_CONTRACT OFF` in these functions.

The latter is the simpler option, and doesn't have any real downside; we can discuss relaxing it later if we want.

================
Comment at: test/builtins/Unit/divdc3_test.c:368
+    {-__DBL_MAX__, -__DBL_MAX__, __DBL_MAX__/2, __DBL_MAX__/2, -2.},
+};
+
----------------
It would be good to build up these tests a bit more. In particular for double, we should include the test cases from Baudin & Smith's "A Robust Complex Division in Scilab" (https://arxiv.org/abs/1210.4539) for double; there are only 10, but they cover most of the hardest-to-get-right finite-result cases for complex division.

They are trying to implement a division with a *componentwise* error-bound. That's a non-goal for compiler-rt, so we shouldn't expect to get them exactly right, but we should be able to get them with small relative error.

Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D63061