Page MenuHomePhabricator

Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double).
ClosedPublic

Authored by lntue on Jul 15 2020, 11:33 PM.

Details

Summary

Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double).

Diff Detail

Event Timeline

lntue created this revision.Jul 15 2020, 11:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2020, 11:33 PM
Herald added a subscriber: mgorny. · View Herald Transcript

Why add support to the TEST macros for this when we have the MPFR macros which I think handle floats better than we can with TEST. Comparisons with floating points are not fun, its why we use the mpfr library in the first place. Is there even a need to use the TEST macros with floats?

libc/utils/CPP/TypeTraits.h
29

It's not your fault, but now that I look at these again, these are wrong. Well at least not exactly analogous to std::is_integral. We need to RemoveCV first. Might not make sense in this patch, but something to note.

libc/utils/UnitTest/Test.cpp
37

Missing trailing . and bellow too

66

This const isn't useful

libc/utils/UnitTest/Test.h
81–82

This is fine because we only use it once, but this is just std::is_arithmetic, we could create a IsArithmetic if you plan to use this pattern more.

libc/utils/testutils/StreamWrapper.cpp
45 ↗(On Diff #278378)

This intuitively doesn't make sense. It's unexpected that a [u]int128 would be printed differently than the smaller width integral type. I don't know what you're trying to do with __uint128_t but I think it's best to create a separate type than do this.

49 ↗(On Diff #278378)

I don't think ULL is necessary. This could also be uint64_t and we could skip the static_casts.
Why const was it supposed to be constexpr?

libc/utils/testutils/StringExtras.h
19–22 ↗(On Diff #278378)

Why not just std::string utohexstr(__uint128_t X, ...)? Otherwise shouldn't this be called u128tohexstr :)

29 ↗(On Diff #278378)

for (; X; X >>= 4)?

Why add support to the TEST macros for this when we have the MPFR macros which I think handle floats better than we can with TEST. Comparisons with floating points are not fun, its why we use the mpfr library in the first place. Is there even a need to use the TEST macros with floats?

Let me give some explanation from my side for this. For non-trivial math functions like trignometric functions, comparing with MPFR result makes sense because we can use MPFR as source of truth/correctness. But, for functions like fmin and friends, comparing with MPFR is an overkill. I agree that comparing floating points in general could be problematic and that is the reason why we did not allow floating comparisons until now. But, if we restrict ourselves to values which have an exact bit representation, then comparisons are precise and deterministic. For example, zero, infinity and small decimals like 10.0, 1.2345 etc. have exact bit representation in all relevant floating point formats Likewise, if a floating point number is actually constructed from the bit representation, then that number has exact bit representation by construction. So, comparing such numbers would be precise.

One can ask, how can we ensure one uses only exact floating point numbers in tests. I don't have a good answer other than saying "code review". May be we can write a clang-tidy driven tool in future. Irrespective, I think this patch improves the readability of the tests. @lntue is the expert here so he might have more to add.

libc/utils/UnitTest/Test.cpp
40

Can we make describeValue return std::string? This way, we will not need to overload llvm::utohexstr.

46

I don't think its of much value to show the decimal value.

47

So, instead of streaming the decimal value, we will just stream out the hex bits with 0x prefix and remove the In hexadecimal:.

56

Same here. I think the decimal value is not of much value. May be in future, when debugging a function like sqrt, seeing the decimal value is useful. But, until then, I would prefer to keep the size of this patch small and skip showing decimal values.

59

Instead of the raw value, for NaN and infinity values, I would add something like this (in parenthesis):

<< '(' << ["+infinity" | "- infinity" | "NaN" ] << ')' << `\n';
63

+1 for this change. Nit: May be call the function explainDifference.

libc/utils/testutils/StreamWrapper.cpp
1 ↗(On Diff #278378)

I prefer keeping this patch to the minimum required and avoid changes in this file.

libc/utils/testutils/StringExtras.h
22 ↗(On Diff #278378)

As I have said above, can we just make describeValue return std::string and put this functionality in the [u]int128 specialization of describeValue?

sivachandra added inline comments.Jul 16 2020, 11:37 AM
libc/utils/CPP/TypeTraits.h
29

Thanks for pointing out. I planned to fix this after adding RemoveCVType but it slipped. @lntue, I will fix this separately and you can take it in.

lntue updated this revision to Diff 279397.Jul 20 2020, 9:09 PM

Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double).

lntue updated this revision to Diff 279418.Jul 20 2020, 9:22 PM
lntue marked 19 inline comments as done.

Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double).

libc/utils/CPP/TypeTraits.h
29

Thanks, I got Siva's updated TypeTraits.h.

libc/utils/UnitTest/Test.h
81–82

Added cpp::IsArithmetic template

libc/utils/testutils/StreamWrapper.cpp
45 ↗(On Diff #278378)

I needed the overload for uint128_t because the llvm::raw_ostream does not support it. Since Siva commented that the decimal value of uint128_t is of no use, and the hexadecimal display of __uint128_t is moved to Test.cpp, this overload is not needed anymore.

49 ↗(On Diff #278378)

Same as the above comment: this overload operator is not needed anymore.

libc/utils/testutils/StringExtras.h
19–22 ↗(On Diff #278378)

Changed the method name to uintToHex to display a fixed number of hexadecimal digits, and moved to Test.cpp, instead of overloading llvm::utohexstr.

lntue added a comment.Jul 20 2020, 9:47 PM

Why add support to the TEST macros for this when we have the MPFR macros which I think handle floats better than we can with TEST. Comparisons with floating points are not fun, its why we use the mpfr library in the first place. Is there even a need to use the TEST macros with floats?

Let me give some explanation from my side for this. For non-trivial math functions like trignometric functions, comparing with MPFR result makes sense because we can use MPFR as source of truth/correctness. But, for functions like fmin and friends, comparing with MPFR is an overkill. I agree that comparing floating points in general could be problematic and that is the reason why we did not allow floating comparisons until now. But, if we restrict ourselves to values which have an exact bit representation, then comparisons are precise and deterministic. For example, zero, infinity and small decimals like 10.0, 1.2345 etc. have exact bit representation in all relevant floating point formats Likewise, if a floating point number is actually constructed from the bit representation, then that number has exact bit representation by construction. So, comparing such numbers would be precise.

One can ask, how can we ensure one uses only exact floating point numbers in tests. I don't have a good answer other than saying "code review". May be we can write a clang-tidy driven tool in future. Irrespective, I think this patch improves the readability of the tests. @lntue is the expert here so he might have more to add.

As Siva mentioned, for math functions that are correctly rounded, the bit patterns of the results are well-defined, and so we can compared their exact bit patterns directly (unless when they are both NaNs, which we can ignore). This would simplify and improve readability for testing such functions. Going further, I plan to add a simple ULP function to allow testing with controlled numbers of error bits.

sivachandra accepted this revision.Jul 20 2020, 10:53 PM

This is very well done. I have left nittiest of nits inline.

libc/utils/UnitTest/Test.cpp
40

Nit: Do we need the flexibility in case? If not, just remove it?

45

Continuing the above comment: Always pass true with a comment attached saying "In upper case" or something similar?

libc/utils/testutils/StreamWrapper.cpp
46 ↗(On Diff #279418)

Do we still need this for this change? May be, but just asking because I couldn't easily locate where.

This revision is now accepted and ready to land.Jul 20 2020, 10:53 PM
abrachet added inline comments.Jul 20 2020, 11:30 PM
libc/utils/UnitTest/Test.cpp
40

Is it more flexible? It's semantically the same as making this std::string uintToHex(__uint128_t X, ...) but inflates the binary.

43

for (auto it = s.rbegin(), end = s.rend(); it != end; ...)

97

Can this be llvm::StringRef instead?

99

This isn't the constructor you're thinking of. You want (OffsetLength, ' ')

118

The RemoveCV isn't necessary anymore

137–138

Because LHS, RHS... are always the same maybe we can create a lambda at the top of this function called fail which just takes the OpString

155

Maybe this and >= need to check if testEQ(...) || LHS < RHS, but FWIW I'm not sure the testEQ is particularly necessary in the first place.

sivachandra added inline comments.Jul 21 2020, 2:15 AM
libc/utils/UnitTest/Test.cpp
40

Not sure I understand your comment. My comment was about LowerCase not being important. We want this function to be a template because it is called with values of different T types but the case seems not required.

99

Ah, good catch!

155

This is a good point and I should have made some notes myself. Even I am not sure what the best approach is. But, testEQ is required because, as explained in the comment there, we want +0.0 and -0.0 to not be equal whereas machine instructions treat them as equal. Next, it is a bit of a slippery slope if we handle only the TEST_LE|GT cases as you suggest. For, what should TEST_GT(+0.0, -0.0) return? We have been treating zeros and NaNs as special inputs and testing them separately anyway. So, what you say about not requiring testEQ has value as the test macros will only be used with non-special values where the specialness of testEQ isn't required. But, will it always be the case? I am not sure what the correct answer is, but I am OK with testEQ as I am viewing it as "experimental". That said, I am also OK if @lntue wants to get rid of testEQ as you suggest. Either way, it is very easy to special case in the tests, and also to debug if something goes wrong. Going forward, I would expect us to pick the approach of least pain.

lntue updated this revision to Diff 279575.Jul 21 2020, 10:41 AM
lntue marked 16 inline comments as done.

Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double).

libc/utils/UnitTest/Test.cpp
40

I also use this to display float, double, and exponent fields. I can move the length N to a function parameter instead to reduce the generated binary.

40

Removed the case.

99

Actually it still works correctly (I deliberately failed a test to see the outputs). I think it was matched with std::string(const char*, size_t, allocator) constructor instead (constructor (4) in https://en.cppreference.com/w/cpp/string/basic_string/basic_string). I change to the other constructor anyway.

155

I would say when we use TEST_LE|GT, we wouldn't care about the difference between +0 and -0. So in that case, the default behavior works fine. But we can update later if a new behavior is needed.

libc/utils/testutils/StreamWrapper.cpp
46 ↗(On Diff #279418)

No, it's not needed anymore. Removed.

abrachet accepted this revision.Jul 21 2020, 10:52 AM
abrachet added inline comments.
libc/utils/UnitTest/Test.cpp
40

I can move the length N to a function parameter instead to reduce the generated binary.

If you want the different number of leading '0''s based on the width of the type then this is fine.

I didn't catch this before but this function has changed semantics between patches. This now returns "00000{hex number}" not just "{hex number}" Is that really what we want or should s be initialized like (Length, '\0')? If it is supposed to be with '\0' then I still think that this should just take a __uint128_t, but if the leading 0's are what we want then this is fine.

abrachet added inline comments.Jul 21 2020, 10:56 AM
libc/utils/UnitTest/Test.cpp
40

Actually I guess it wouldn't need to be initialized like that, that would just return "", oops. But last function returned a substring and didn't have any leading 0's and no need for the size of the type

This revision was automatically updated to reflect the committed changes.