This is an archive of the discontinued LLVM Phabricator instance.

[libc] Move implementations of cosf, sinf, sincosf to src/math directory.
ClosedPublic

Authored by sivachandra on Mar 25 2020, 10:06 PM.

Details

Summary

NFC intended in the implementaton. Only mechanical changes to fit the LLVM
libc implementation standard have been done.

Math testing infrastructure has been added. This infrastructure compares the
results produced by the libc with the high precision results from MPFR.
Tests making use of this infrastructure have been added for cosf, sinf and
sincosf.

Diff Detail

Event Timeline

sivachandra created this revision.Mar 25 2020, 10:06 PM

This patch is not ready for review yet. For one, it does not have any tests. I am only sharing to showcase the move early.

sivachandra edited the summary of this revision. (Show Details)Mar 25 2020, 10:09 PM
sivachandra edited the summary of this revision. (Show Details)
abrachet added inline comments.
libc/src/math/CMakeLists.txt
10

Will this properly propagate to targets which depend on math_utils?

libc/src/math/cosf.cpp
20–22

Maybe we could remove the two spaces after the period. This isn't common in the rest of our comments

libc/src/math/math_utils.h
2

Should this file have a comment header?

libc/src/math/sincosf.h
2

sinf -> sincosf

libc/src/math/sincosf_utils.h
34–37

We can put the data from sincosf_data and just make them constexpr perhaps.

This patch is not ready for review yet. For one, it does not have any tests. I am only sharing to showcase the move early.

It's probably worth discussing how much we expect we will alter these files. Is it fine to just accept that we didn't write them and that they will just be very different from the rest of libc?

It's probably worth discussing how much we expect we will alter these files. Is it fine to just accept that we didn't write them and that they will just be very different from the rest of libc?

While I do intend to make any functional changes in this move patch, we absolutely want the code to follow our implementation standards, just like the rest of the code. So, thanks for already pointing out the items missed. For now, all I have done was to run clang-format. I will fix the rest with a new upload.

It's probably worth discussing how much we expect we will alter these files. Is it fine to just accept that we didn't write them and that they will just be very different from the rest of libc?

While I do intend to make any functional changes in this move patch, we absolutely want the code to follow our implementation standards, just like the rest of the code. So, thanks for already pointing out the items missed. For now, all I have done was to run clang-format. I will fix the rest with a new upload.

I mean't to say, "do NOT intend".

Won't deleting the files in the AOR directory cause the make check on the buildbots to fail? It might make sense to leave the files and delete them once all functions have been migrated?

Won't deleting the files in the AOR directory cause the make check on the buildbots to fail? It might make sense to leave the files and delete them once all functions have been migrated?

Yes, it will. I plan to move the tests also in a way that make check does not break.

Not yet ready for review. Changes in this update:

  • Add test infrastructure for math functions which uses MPFR.
  • Add tests for cosf and sinf

This is not yet ready for review. I have added bunch of tests but more tests need to be ported from the AOR directory. Also, the test infrastructure needs a lot of comments which I will add in my next update.

  • All tests I planned to add in this round are now added.
  • ULP related tests have not been ported yet, but that can be done in a different round instead of cramming up this patch with everything.
  • Added comments which I planned to add to explain the various pieces of the test infrastructure.
sivachandra edited the summary of this revision. (Show Details)Apr 10 2020, 5:38 PM
sivachandra edited the summary of this revision. (Show Details)

Rebase.

abrachet added inline comments.Apr 10 2020, 7:33 PM
libc/src/__support/common.h.def
16

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.

libc/src/math/cosf.cpp
59–61

Remove this else, just return invalidf(y)

libc/src/math/sincosf.cpp
13

Should we use " instead of <

libc/src/math/sincosf.h
1

Only 2 -

9

SINCOSF

libc/src/math/sincosf_utils.h
9

SINCOSF_UTILS

74

I think we can declare all of these where they are initialized

80

Not sure why they choose the name x7, isn't x5 more appropriate.

84

No need for this else

106

Declare r down here.

libc/src/math/sinf.cpp
33–35

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.

63

Remove this else

libc/test/src/math/cosf_test.cpp
30

; -> .

37–39

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);

71–82

Is this meant to be here?

libc/test/src/math/sincosf_test.cpp
30

. not ;

libc/test/src/math/sinf_test.cpp
30

; -> .

libc/utils/MPFRWrapper/MPFRUtils.cpp
44

for (int exponent = -t.basePrecision; bitMask ... maybe

libc/utils/MPFRWrapper/MPFRUtils.h
38–39

This could be a namespace or at least a struct

sivachandra marked 15 inline comments as done.
  • Address comments.
  • Add tests for small input values to sinf/cosf/sincosf functions.

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.

libc/src/math/CMakeLists.txt
10

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.

libc/src/math/sinf.cpp
33–35

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.

libc/test/src/math/cosf_test.cpp
37–39

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.

71–82

Yup, I was playing with tolerance and forgot to un-comment aferwards.

  • Address one missed comment.
sivachandra marked an inline comment as done.Apr 13 2020, 10:30 PM

There are few hexadecimal floating literals in this patch which moved along with the AOR implementations. They trigger warnings of this form:

warning: hexadecimal floating literals are a C++17 feature [-Wc++17-extensions]

I think I can fix them by using their bit patterns as is already done in other places. Will do so in the next pass.

abrachet accepted this revision.Apr 14 2020, 10:00 PM

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.

Sounds good. LGTM

This revision is now accepted and ready to land.Apr 14 2020, 10:00 PM
phosek accepted this revision.Apr 16 2020, 1:11 AM

LGTM, I had the same concern as @abrachet regarding some of the readability bits (curly braces, minimizing variable scopes) but keeping the code as close to original SGTM as well. If we decide to make those changes, we could probably automate them later using a tool like clang-tidy.

This revision was automatically updated to reflect the committed changes.
libc/AOR_v20.02/math/sinf.c