[libc-commits] [PATCH] D76825: [libc] Move implementations of cosf, sinf, sincosf to src/math directory.

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/src/__support/common.h.def:16
+#define unlikely(x) __builtin_expect (x, 0)
+#define UNUSED __attribute__((unused))
+
----------------
I think it might be more common in llvm to cast to void rather than use a macro like this. But I don't have a strong preference.


================
Comment at: libc/src/math/cosf.cpp:59-61
+  } else {
+    return invalidf(y);
+  }
----------------
Remove this else, just return invalidf(y)


================
Comment at: libc/src/math/sincosf.cpp:13
+#include "src/__support/common.h"
+#include <include/math.h>
+
----------------
Should we use " instead of <


================
Comment at: libc/src/math/sincosf.h:1
+//===--- Implementation header for sincosf ----------------------*- C++ -*-===//
+//
----------------
Only 2 `-`


================
Comment at: libc/src/math/sincosf.h:9
+
+#ifndef LLVM_LIBC_SRC_MATH_SINF_H
+#define LLVM_LIBC_SRC_MATH_SINF_H
----------------
SINCOSF


================
Comment at: libc/src/math/sincosf_utils.h:9
+
+#ifndef LLVM_LIBC_SRC_MATH_SINCOSF_H
+#define LLVM_LIBC_SRC_MATH_SINCOSF_H
----------------
SINCOSF_UTILS


================
Comment at: libc/src/math/sincosf_utils.h:74
+static inline float sinf_poly(double x, double x2, const sincos_t *p, int n) {
+  double x3, x4, x6, x7, s, c, c1, c2, s1;
+
----------------
I think we can declare all of these where they are initialized


================
Comment at: libc/src/math/sincosf_utils.h:80
+
+    x7 = x3 * x2;
+    s = x + x3 * p->s1;
----------------
Not sure why they choose the name x7, isn't x5 more appropriate.


================
Comment at: libc/src/math/sincosf_utils.h:84
+    return s + x7 * s1;
+  } else {
+    x4 = x2 * x2;
----------------
No need for this else


================
Comment at: libc/src/math/sincosf_utils.h:106
+  // This avoids inaccuracies introduced by truncating negative values.
+  r = x * p->hpi_inv;
+  int n = ((int32_t)r + 0x800000) >> 24;
----------------
Declare r down here.


================
Comment at: libc/src/math/sinf.cpp:33-35
+      if (unlikely(abstop12(y) < abstop12(0x1p-126f)))
+        // Force underflow for tiny y.
+        force_eval_float(s);
----------------
I don't want to comment on the substance of the code too much, but I'll risk making a fool out of myself for this one. These lines logically don't make sense if we are in the first if then we will return y and s has no impact on that. So I wonder what purpose these lines serve.


================
Comment at: libc/src/math/sinf.cpp:63
+    return sinf_poly(x * s, x * x, p, n);
+  } else
+    return invalidf(y);
----------------
Remove this else


================
Comment at: libc/test/src/math/cosf_test.cpp:30
+// 12 additional bits of precision over the base precision of a |float|
+// value;
+static constexpr Tolerance tolerance{Tolerance::floatPrecision, 12,
----------------
; -> .


================
Comment at: libc/test/src/math/cosf_test.cpp:37-39
+  EXPECT_TRUE(FloatBits::isQNan(
+      as_uint32_bits(__llvm_libc::cosf(as_float(FloatBits::QNan)))));
+  EXPECT_EQ(llvmlibc_errno, 0);
----------------
These could become `EXPECT_THAT(Succeeds(FloatBits::isQNan(as_uint32_bits(__llvm_libc::cosf(as_float(FloatBits::QNan))))), true);` and no longer need the `EXPECT_EQ(llvmlibc_errno, 0);`


================
Comment at: libc/test/src/math/cosf_test.cpp:71-82
+/*
+TEST(CosfTest, InFloatRange) {
+  constexpr uint32_t count = 1000000;
+  constexpr uint32_t step = UINT32_MAX / count;
+  for (uint32_t i = 0, v = 0; i <= count; ++i, v += step) {
+    float x = as_float(v);
+    if (isnan(x) || isinf(x))
----------------
Is this meant to be here?


================
Comment at: libc/test/src/math/sincosf_test.cpp:30
+// 12 additional bits of precision over the base precision of a |float|
+// value;
+static constexpr Tolerance tolerance{Tolerance::floatPrecision, 12,
----------------
`.` not `;`


================
Comment at: libc/test/src/math/sinf_test.cpp:30
+// 12 additional bits of precision over the base precision of a |float|
+// value;
+static constexpr Tolerance tolerance{Tolerance::floatPrecision, 12,
----------------
; -> .


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:44
+    int exponent = -t.basePrecision;
+    for (; bitMask > 0; bitMask >>= 1) {
+      --exponent;
----------------
`for (int exponent = -t.basePrecision; bitMask ...` maybe


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.h:38-39
+
+class MPFR {
+public:
+  // Return true if |libcOutput| is within the tolerance |t| of the cos(x)
----------------
This could be a namespace or at least a struct


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76825