[libc-commits] [PATCH] D133550: [libc][math] Implement acosf function correctly rounded for all rounding modes.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/math/generic/asinf.cpp:17
 
 #include <errno.h>
 
----------------
orex wrote:
> Please delete this. It is not needed here. My fault.
We do need this to set `EDOM` for out of range inputs.  `errno.h` header is generated separately (it's kind of complete I think) and does not depend on `math.h`, so it's safe to include and use in math entrypoints.  That's not true for `math.h` constants though, as we are building that header's implementation here.


================
Comment at: libc/src/math/generic/inv_trigf_utils.h:95
+//                 [|1, D...|], [0, 0.5]);
+constexpr double ASIN_COEFFS[10] = {0x1.5555555540fa1p-3, 0x1.333333512edc2p-4,
+                                    0x1.6db6cc1541b31p-5, 0x1.f1caff324770ep-6,
----------------
orex wrote:
> Don't you think that it is better to put this array to cpp file?
This is a small table that we do want to inline, and we don't take its address anywhere, so by leaving its definition in the header, it does improve the performance.  I got the reciprocal throughput of 24 for leaving its definition here, vs 26 for putting its definition in the cpp file and providing external linkage in the header.


================
Comment at: libc/test/src/math/exhaustive/acosf_test.cpp:38
 
 // Range: [0, Inf];
 static const uint32_t POS_START = 0x0000'0000U;
----------------
orex wrote:
> Do you really need this test until inf? Out of range values can be covered by unittests?
I think it's better to leave the whole range in the committed tests to safe guard against future changes.  Ideally (and soon) we will run these tests automatically with the CI's.  For manual testing, we can always restrict the the range that we are interested.  The main reason that I didn't make other exhaustive tests running full range is that the `ulp` function comparing to `mpfr` outputs did not handle `NaN` properly.  It was fixed in https://reviews.llvm.org/D133400, so I think it's better to just test everything in these exhaustive tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133550