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

[lntue]

Summary

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

[abrachet]

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

[sivachandra]

In D83931#2155091, @abrachet wrote:

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]

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]

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

[lntue]

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]

In D83931#2156530, @sivachandra wrote:

In D83931#2155091, @abrachet wrote:

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]

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.

[abrachet]

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]

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]

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]

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]

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

libc/utils/CPP/TypeTraits.h

 struct TrueValue {
   static constexpr bool Value = true;
 };
 
 struct FalseValue {
   static constexpr bool Value = false;
 };
 
 template <typename T> struct TypeIdentity { typedef T Type; };
 
 template <typename T1, typename T2> struct IsSame : public FalseValue {};
 template <typename T> struct IsSame<T, T> : public TrueValue {};
 template <typename T1, typename T2>
 static constexpr bool IsSameV = IsSame<T1, T2>::Value;
 
 template <typename T> struct RemoveCV : public TypeIdentity<T> {};
 template <typename T> struct RemoveCV<const T> : public TypeIdentity<T> {};
 template <typename T> struct RemoveCV<volatile T> : public TypeIdentity<T> {};
 template <typename T>
 struct RemoveCV<const volatile T> : public TypeIdentity<T> {};
 
 template <typename T> using RemoveCVType = typename RemoveCV<T>::Type;
 
 template <typename Type> struct IsIntegral {
   using TypeNoCV = RemoveCVType<Type>;
   static constexpr bool Value =
       IsSameV<char, TypeNoCV> || IsSameV<signed char, TypeNoCV> ||
       IsSameV<unsigned char, TypeNoCV> || IsSameV<short, TypeNoCV> ||
       IsSameV<unsigned short, TypeNoCV> || IsSameV<int, TypeNoCV> ||
       IsSameV<unsigned int, TypeNoCV> || IsSameV<long, TypeNoCV> ||
       IsSameV<unsigned long, TypeNoCV> || IsSameV<long long, TypeNoCV> ||
-      IsSameV<unsigned long long, TypeNoCV> || IsSameV<bool, TypeNoCV>;
+      IsSameV<unsigned long long, TypeNoCV> || IsSameV<bool, TypeNoCV> ||
+      IsSameV<__uint128_t, TypeNoCV>;
 };
 
 template <typename T> struct IsPointerTypeNoCV : public FalseValue {};
 template <typename T> struct IsPointerTypeNoCV<T *> : public TrueValue {};
 template <typename T> struct IsPointerType {
   static constexpr bool Value = IsPointerTypeNoCV<RemoveCVType<T>>::Value;
 };
 
 template <typename Type> struct IsFloatingPointType {
   using TypeNoCV = RemoveCVType<Type>;
   static constexpr bool Value = IsSame<float, TypeNoCV>::Value ||
                                 IsSame<double, TypeNoCV>::Value ||
                                 IsSame<long double, TypeNoCV>::Value;
 };
 
+template <typename Type> struct IsArithmetic {
+  static constexpr bool Value =
+      IsIntegral<Type>::Value || IsFloatingPointType<Type>::Value;
+};
+
 } // namespace cpp
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_UTILS_CPP_TYPETRAITS_H

libc/utils/UnitTest/Test.h

protected:
   // We make use of a template function, with |LHS| and |RHS| as explicit
   // parameters, for enhanced type checking. Other gtest like unittest
   // frameworks have a similar function which takes a boolean argument
   // instead of the explicit |LHS| and |RHS| arguments. This boolean argument
   // is the result of the |Cond| operation on |LHS| and |RHS|. Though not bad,
   // |Cond| on mismatched |LHS| and |RHS| types can potentially succeed because
   // of type promotion.
   template <typename ValType,
-            cpp::EnableIfType<cpp::IsIntegral<ValType>::Value, ValType> = 0>
+            cpp::EnableIfType<cpp::IsArithmetic<ValType>::Value, int> = 0>
   static bool test(RunContext &Ctx, TestCondition Cond, ValType LHS,
                    ValType RHS, const char *LHSStr, const char *RHSStr,
                    const char *File, unsigned long Line) {
     return internal::test(Ctx, Cond, LHS, RHS, LHSStr, RHSStr, File, Line);
   }
 
   template <
       typename ValType,
       cpp::EnableIfType<cpp::IsPointerType<ValType>::Value, ValType> = nullptr>

libc/utils/UnitTest/Test.cpp

 //===-- Implementation of the base class for libc unittests ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Test.h"
 
+#include "utils/FPUtil/FPBits.h"
 #include "utils/testutils/ExecuteFunction.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace __llvm_libc {
 namespace testing {
 
 // This need not be a class as all it has is a single read-write state variable.
 // But, we make it class as then its implementation can be hidden from the
 // header file.
 class RunContext {
 public:
   enum RunResult { Result_Pass = 1, Result_Fail = 2 };
 
   RunResult status() const { return Status; }
 
   void markFail() { Status = Result_Fail; }
 
 private:
   RunResult Status = Result_Pass;
 };
 
 namespace internal {
 
+// Display the first N hexadecimal digits of an integer in upper case.
+template <typename T>
+cpp::EnableIfType<cpp::IsIntegral<T>::Value, std::string>
+uintToHex(T X, size_t Length = sizeof(T) * 2) {
+  std::string s(Length, '0');
+
+  for (auto it = s.rbegin(), end = s.rend(); it != end; ++it, X >>= 4) {
+    unsigned char Mod = static_cast<unsigned char>(X) & 15;
+    *it = llvm::hexdigit(Mod, true);
+  }
+
+  return s;
+}
+
+// When the value is not floating-point type, just display it as normal.
+template <typename ValType>
+cpp::EnableIfType<!cpp::IsFloatingPointType<ValType>::Value, std::string>
+describeValue(ValType Value) {
+  return std::to_string(Value);
+}
+
+template <> std::string describeValue<llvm::StringRef>(llvm::StringRef Value) {
+  return std::string(Value);
+}
+
+// When the value is __uint128_t, also show its hexadecimal digits.
+// Using template to force exact match, prevent ambiguous promotion.
+template <> std::string describeValue<__uint128_t>(__uint128_t Value) {
+  return "0x" + uintToHex(Value);
+}
+
+// When the value is a floating point type, also show its sign | exponent |
+// mantissa.
+template <typename ValType>
+cpp::EnableIfType<cpp::IsFloatingPointType<ValType>::Value, std::string>
+describeValue(ValType Value) {
+  fputil::FPBits<ValType> Bits(Value);
+
+  if (Bits.isNaN()) {
+    return "(NaN)";
+  } else if (Bits.isInf()) {
+    return Bits.sign ? "(-Infinity)" : "(+Infinity)";
+  } else {
+    constexpr int ExponentWidthInHex =
+        (fputil::ExponentWidth<ValType>::value - 1) / 4 + 1;
+    constexpr int MantissaWidthInHex =
+        (fputil::MantissaWidth<ValType>::value - 1) / 4 + 1;
+
+    return std::string("Sign: ") + (Bits.sign ? '1' : '0') + ", Exponent: 0x" +
+           uintToHex<uint16_t>(Bits.exponent, ExponentWidthInHex) +
+           ", Mantissa: 0x" +
+           uintToHex<typename fputil::FPBits<ValType>::UIntType>(
+               Bits.mantissa, MantissaWidthInHex);
+  }
+}
+
+template <typename ValType>
+void explainDifference(ValType LHS, ValType RHS, const char *LHSStr,
+                       const char *RHSStr, const char *File, unsigned long Line,
+                       llvm::StringRef OpString) {
+  size_t OffsetLength = OpString.size() > 2 ? OpString.size() - 2 : 0;
+  std::string Offset(OffsetLength, ' ');
+
+  llvm::outs() << File << ":" << Line << ": FAILURE\n"
+               << Offset << "Expected: " << LHSStr << '\n'
+               << Offset << "Which is: " << describeValue(LHS) << '\n'
+               << "To be " << OpString << ": " << RHSStr << '\n'
+               << Offset << "Which is: " << describeValue(RHS) << '\n';
+}
+
+template <typename ValType>
+cpp::EnableIfType<!cpp::IsFloatingPointType<ValType>::Value, bool>
+testEQ(ValType LHS, ValType RHS) {
+  return LHS == RHS;
+}
+
+// For floating points, we consider all NaNs are equal, and +0.0 is not equal to
+// -0.0.
+template <typename ValType>
+cpp::EnableIfType<cpp::IsFloatingPointType<ValType>::Value, bool>
+testEQ(ValType LHS, ValType RHS) {
+  fputil::FPBits<ValType> LHSBits(LHS), RHSBits(RHS);
+
+  return (LHSBits.isNaN() && RHSBits.isNaN()) ||
+         (LHSBits.bitsAsUInt() == RHSBits.bitsAsUInt());
+}
+
 template <typename ValType>
 bool test(RunContext &Ctx, TestCondition Cond, ValType LHS, ValType RHS,
           const char *LHSStr, const char *RHSStr, const char *File,
           unsigned long Line) {
+  auto ExplainDifference = [=](llvm::StringRef OpString) {
+    explainDifference(LHS, RHS, LHSStr, RHSStr, File, Line, OpString);
+  };
+
   switch (Cond) {
   case Cond_EQ:
-    if (LHS == RHS)
+    if (testEQ(LHS, RHS))
       return true;
 
     Ctx.markFail();
-    llvm::outs() << File << ":" << Line << ": FAILURE\n"
-                 << "      Expected: " << LHSStr << '\n'
-                 << "      Which is: " << LHS << '\n'
-                 << "To be equal to: " << RHSStr << '\n'
-                 << "      Which is: " << RHS << '\n';
-
+    ExplainDifference("equal to");
     return false;
   case Cond_NE:
-    if (LHS != RHS)
+    if (!testEQ(LHS, RHS))
       return true;
 
     Ctx.markFail();
-    llvm::outs() << File << ":" << Line << ": FAILURE\n"
-                 << "          Expected: " << LHSStr << '\n'
-                 << "          Which is: " << LHS << '\n'
-                 << "To be not equal to: " << RHSStr << '\n'
-                 << "          Which is: " << RHS << '\n';
+    ExplainDifference("not equal to");
     return false;
   case Cond_LT:
     if (LHS < RHS)
       return true;
 
     Ctx.markFail();
-    llvm::outs() << File << ":" << Line << ": FAILURE\n"
-                 << "       Expected: " << LHSStr << '\n'
-                 << "       Which is: " << LHS << '\n'
-                 << "To be less than: " << RHSStr << '\n'
-                 << "       Which is: " << RHS << '\n';
+    ExplainDifference("less than");
     return false;
   case Cond_LE:
     if (LHS <= RHS)
       return true;
 
     Ctx.markFail();
-    llvm::outs() << File << ":" << Line << ": FAILURE\n"
-                 << "                   Expected: " << LHSStr << '\n'
-                 << "                   Which is: " << LHS << '\n'
-                 << "To be less than or equal to: " << RHSStr << '\n'
-                 << "                   Which is: " << RHS << '\n';
+    ExplainDifference("less than or equal to");
     return false;
   case Cond_GT:
     if (LHS > RHS)
       return true;
 
     Ctx.markFail();
-    llvm::outs() << File << ":" << Line << ": FAILURE\n"
-                 << "          Expected: " << LHSStr << '\n'
-                 << "          Which is: " << LHS << '\n'
-                 << "To be greater than: " << RHSStr << '\n'
-                 << "          Which is: " << RHS << '\n';
+    ExplainDifference("greater than");
     return false;
   case Cond_GE:
     if (LHS >= RHS)
       return true;
 
     Ctx.markFail();
-    llvm::outs() << File << ":" << Line << ": FAILURE\n"
-                 << "                      Expected: " << LHSStr << '\n'
-                 << "                      Which is: " << LHS << '\n'
-                 << "To be greater than or equal to: " << RHSStr << '\n'
-                 << "                      Which is: " << RHS << '\n';
+    ExplainDifference("greater than or equal to");
     return false;
   default:
     Ctx.markFail();
     llvm::outs() << "Unexpected test condition.\n";
     return false;
   }
 }
 
template bool Test::test<bool, 0>(RunContext &Ctx, TestCondition Cond, bool LHS,
                                   const char *RHSStr, const char *File,
                                   unsigned long Line);
 
 template bool Test::test<unsigned long long, 0>(
     RunContext &Ctx, TestCondition Cond, unsigned long long LHS,
     unsigned long long RHS, const char *LHSStr, const char *RHSStr,
     const char *File, unsigned long Line);
 
+template bool Test::test<__uint128_t, 0>(RunContext &Ctx, TestCondition Cond,
+                                         __uint128_t LHS, __uint128_t RHS,
+                                         const char *LHSStr, const char *RHSStr,
+                                         const char *File, unsigned long Line);
+
+template bool Test::test<float, 0>(RunContext &Ctx, TestCondition Cond,
+                                   float LHS, float RHS, const char *LHSStr,
+                                   const char *RHSStr, const char *File,
+                                   unsigned long Line);
+
+template bool Test::test<double, 0>(RunContext &Ctx, TestCondition Cond,
+                                    double LHS, double RHS, const char *LHSStr,
+                                    const char *RHSStr, const char *File,
+                                    unsigned long Line);
+
+template bool Test::test<long double, 0>(RunContext &Ctx, TestCondition Cond,
+                                         long double LHS, long double RHS,
+                                         const char *LHSStr, const char *RHSStr,
+                                         const char *File, unsigned long Line);
+
 bool Test::testStrEq(RunContext &Ctx, const char *LHS, const char *RHS,
                      const char *LHSStr, const char *RHSStr, const char *File,
                      unsigned long Line) {
   return internal::test(Ctx, Cond_EQ, llvm::StringRef(LHS),
                         llvm::StringRef(RHS), LHSStr, RHSStr, File, Line);
 }
 
 bool Test::testStrNe(RunContext &Ctx, const char *LHS, const char *RHS,