[libc-commits] [PATCH] D129275: [libc][math] Added coshf function.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/FPUtil/FPBits.h:69
 
-  void set_unbiased_exponent(UIntType expVal) {
+  inline void set_unbiased_exponent(UIntType expVal) {
     expVal = (expVal << (FloatProp::MANTISSA_WIDTH)) & FloatProp::EXPONENT_MASK;
----------------
Why do we need extra `inline`'s here?  They should be implicit for class methods with definitions in the headers?


================
Comment at: libc/src/math/generic/coshf.cpp:43-45
+  double ep = fputil::multiply_add(ep_p.mult_exp, ep_p.r, ep_p.mult_exp - 1.0) +
+              fputil::multiply_add(ep_m.mult_exp, ep_m.r, ep_m.mult_exp - 1.0);
+  return 0.5 * ep + 1.0;
----------------
Add comments about your expanded formula / computations:
```
exp(x) = ep_p.mult_exp * (ep_p.r + 1)
exp(-x) = ep_m.mult_exp * (ep_m.r + 1)
cosh(x) = (exp(x) + exp(-x)) / 2 = ...
```
In the evaluation, it looks like there is ` (... - 1.0) + (... - 1.0)` followed by `(0.5 * ...) + 1.0` so all the 1.0's are actually cancelled.  Maybe this can be simplified to:
```
ep = fputil::multiply_add(ep_p.mult_exp, ep_p.r, ep_p.mult_exp) +
     fputil::multiply_add(ep_m.mult_exp, ep_m.r, ep_m.mult_exp);
return 0.5 * ep;
```
And the `0.5 * ep` can even be dropped with an update in `exp_eval`.


================
Comment at: libc/test/src/math/CMakeLists.txt:1317
+  NEED_MPFR
+  NO_RUN_POSTBUILD
+  SUITE
----------------
Unit test doesn't need `NO_RUN_POSTBUILD`, unless you only want to run it manually.


================
Comment at: libc/test/src/math/coshf_test.cpp:64
+      continue;
+    ASSERT_MPFR_MATCH(mpfr::Operation::Cosh, x, __llvm_libc::coshf(x), 1.0);
+  }
----------------
0.5 for tolerance?


================
Comment at: libc/test/src/math/coshf_test.cpp:71
+  float result = __llvm_libc::coshf(x);
+  EXPECT_MPFR_MATCH(mpfr::Operation::Cosh, x, result, 1.0);
+  EXPECT_FP_EQ(1.0f, result);
----------------
0.5 for tolerance?


================
Comment at: libc/test/src/math/coshf_test.cpp:76
+  result = __llvm_libc::coshf(x);
+  EXPECT_MPFR_MATCH(mpfr::Operation::Cosh, x, result, 1.0);
+  EXPECT_FP_EQ(1.0f, result);
----------------
0.5 for tolerance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129275