This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add ULP function to MPFRNumber class to test correctly rounded functions such as SQRT, FMA.
ClosedPublic

Authored by lntue on Jul 27 2020, 10:40 PM.

Details

Summary

Add ULP function to MPFRNumber class to test correctly rounded functions.

Diff Detail

Event Timeline

lntue created this revision.Jul 27 2020, 10:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2020, 10:40 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 27 2020, 11:50 PM
Harbormaster failed remote builds in B65957: Diff 281114!
lntue updated this revision to Diff 281354.Jul 28 2020, 2:02 PM

[libc] Add ULP function to MPFRNumber class to test correctly rounded functions such as SQRT, FMA.

lntue requested review of this revision.Jul 28 2020, 2:02 PM
lntue updated this revision to Diff 281723.Jul 29 2020, 1:37 PM

Fix explicit specialization in class scope.

The functionality LGTM. May be a few more comments would have helped me read this faster, but never mind for now. I am want to spend a little more time the layering of FPUtils, MPFRWrapper and UnitTest. Will come back soon with more comments.

We currently have UnitTest depend on FPUtil. I think that an creates unnecessary dependency for tests which do not need FPUtil for testing. So, a plan could be:

Instead of EXPECT_EQ and friends for floating point numbers, we will decouple UnitTest from FPUtil using matchers implemented in FPUtil. Say, EXPECT_FP_MATCH etc.

WDYT? If you concur, then you need not do the work. I will do it, and you can absorb those changes into this patch.

lntue added a comment.Aug 5 2020, 12:16 PM

We currently have UnitTest depend on FPUtil. I think that an creates unnecessary dependency for tests which do not need FPUtil for testing. So, a plan could be:

Instead of EXPECT_EQ and friends for floating point numbers, we will decouple UnitTest from FPUtil using matchers implemented in FPUtil. Say, EXPECT_FP_MATCH etc.

WDYT? If you concur, then you need not do the work. I will do it, and you can absorb those changes into this patch.

That sounds good to me. Let me know when it's ready. Thanks!

lntue updated this revision to Diff 285254.Aug 12 2020, 10:44 PM

Merge with reviews.llvm.org/D85486

sivachandra added inline comments.Aug 13 2020, 12:06 AM
libc/utils/MPFRWrapper/MPFRUtils.cpp
172

Few comments about documentation.

  1. I think this function assumes that the exponent of input matches that of the MPFR number. If yes, document it here?
  2. Explain why a double return value is good enough?
  3. Lastly, is the name ulp of this method correct? I guess, because you defined ULP above as a ratio, the name ulp is correct and is OK as is.
207

Instead of non-member functions, can we replace the existing methods with a template method:

template <typename T>
T as() const;

We will still need specializations like below though.

237

In what circumstances would a rounded MPFR result help understand a test failure?

240

Is plural errors correct here?

lntue marked an inline comment as done.Aug 13 2020, 8:09 AM
lntue added inline comments.
libc/utils/MPFRWrapper/MPFRUtils.cpp
237

When the ULP error is exactly 0.5, the ULP value itself is not enough to check if the rounding is correct. This should never happen with SQRT function, but other one like HYPOT or FMA might be able to hit exactly 0.5 ULP.

lntue updated this revision to Diff 285904.Aug 16 2020, 2:32 PM

Correct the eps when the input is a power of 2, and add a check for correct rounding to even when it is a tie.

lntue updated this revision to Diff 286320.Aug 18 2020, 9:38 AM

Update comments

lntue marked 3 inline comments as done.Aug 18 2020, 9:40 AM
lntue added inline comments.
libc/utils/MPFRWrapper/MPFRUtils.cpp
172

I updated the comments to reflect the expectations.

237

I updated the test to reflect the case when there is a tie in rounded, i.e. ULP error == 0.5, we expect it to be rounded to even

sivachandra accepted this revision.Aug 18 2020, 10:14 AM
sivachandra added inline comments.
libc/utils/MPFRWrapper/MPFRUtils.cpp
176

May be reword it as:

We expect that this value and the value to be compared (the |input| argument) are reasonably close ...
This revision is now accepted and ready to land.Aug 18 2020, 10:14 AM
lntue updated this revision to Diff 286353.Aug 18 2020, 10:48 AM
lntue marked an inline comment as done.

Update comments.

This revision was landed with ongoing or failed builds.Aug 18 2020, 10:52 AM
This revision was automatically updated to reflect the committed changes.