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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Thanks a lot for the detailed review.

I did not address some of the comments. The plan is to quickly absorb the AOR code into the normal LLVM-libc way and try to bring AOR developers to work with LLVM-libc. To that extant, I want to keep their structuring as intact as possible so that they do not have to go through a learning phase to understand their own code. We do want the higher-level structuring to adhere to LLVM libc's style so I tried to get that in shape with this change.



================
Comment at: libc/src/math/CMakeLists.txt:9
+  DEPENDS
+    __errno_location
+)
----------------
abrachet wrote:
> Will this properly propagate to targets which depend on math_utils?
That's a good point. It does not and we have to add the entrypoint deps explicitly. Not ideal but I will try to work something out with my change to propagate entrypoint deps.


================
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);
----------------
abrachet wrote:
> 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.
Neither do I know. Like I have explained, I want to keep it as is so I that we don't increase the unfamiliarity for Arm developers.


================
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);
----------------
abrachet wrote:
> 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);`
I started with that but I found that so much more unreadable in this case. That said, I want to do better here. For example, if something fails, we aren't printing information about what two values/bit patterns were compared. I have some ideas which I want to start incorporating with more functions that I convert.


================
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))
----------------
abrachet wrote:
> Is this meant to be here?
Yup, I was playing with tolerance and forgot to un-comment aferwards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76825