[libc] Improve information printed on failure of a math test which uses MPFR.

[sivachandra]

Summary

A new test matcher class MPFRMatcher is added along with helper macros
EXPECT|ASSERT_MPFR_MATCH.

New type traits classes RemoveCV and IsFloatingPointType have been
added and used to implement the above class and its helpers.

[sivachandra]

Remove the long double instantiation.
Can add it back when we deal with long double in future.

[abrachet]

I don't want to make any suggestions yet because I don't fully understand why we need a MPFR_TEST. Would you mind clarifying the need for TEST_WITH_BASE_CLASS? Instead of MPFR_TEST could we have TEST_EXTENSION's first argument take a class with compare, would that simplify it? I'm by no means a gtest expert, I'm not sure how someone would use gtest to fill these kinds of needs. Maybe they would make their own macros which end up calling FAIL or using matchers. We aren't limited to that though so perhaps these would end up being more complicated than this solution.

libc/utils/CPP/TypeTraits.h

50–52

type -> Type

On that note, we could make an analog to std::type_identity and make this
template <typename T> struct RemoveCV : TypeIdentity<T> {};. No strong preference though.

libc/utils/MPFRWrapper/MPFRUtils.cpp

26–27

Is there a reason that this is not just a static method of MPFRNumber?

91

Might as well use mpfr_snprintf?

121–123

We can just keep streaming instead of ending the statement and calling llvm::outs() again perhaps? That would be stylistically similar to what we have in Test.cpp

libc/utils/MPFRWrapper/MPFRUtils.h

72–74

We aren't enabling another specialization when this isn't true so could we just make this a static_assert?

[sivachandra]

In D79256#2016625, @abrachet wrote:

I don't want to make any suggestions yet because I don't fully understand why we need a MPFR_TEST. Would you mind clarifying the need for TEST_WITH_BASE_CLASS? Instead of MPFR_TEST could we have TEST_EXTENSION's first argument take a class with compare, would that simplify it? I'm by no means a gtest expert, I'm not sure how someone would use gtest to fill these kinds of needs. Maybe they would make their own macros which end up calling FAIL or using matchers. We aren't limited to that though so perhaps these would end up being more complicated than this solution.

Huh! I totally missed that we have matchers now with which we can do the exact same thing as in this patch. Will change it to use the matcher API and update soon.

[sivachandra]

• Use a matcher instead of special test class.
• Addressed relevant comments.

[sivachandra]

libc/utils/CPP/TypeTraits.h

50–52

At the least it avoid typos like the one you found. So, done.

[abrachet]

libc/utils/CPP/TypeTraits.h

57

I believe that typename shouldn't be required and RemoveCV<T>::Type should be enough.

libc/utils/MPFRWrapper/MPFRUtils.cpp

11

No longer needed, I believe

29–41

Do these need to be enable if specializations could they just be overloads?

43–44

Could this also be just a static_assert?

libc/utils/MPFRWrapper/MPFRUtils.h

75–77

Can this one also be static_assert?

[sivachandra]

Address comments.

[sivachandra]

libc/utils/CPP/TypeTraits.h

57

RemoveCV<T>::Type is a dependent type and should require typename? I tried your suggestion and that is what the compiler also tells me:

TypeTraits.h:57:44: error: missing 'typename' prior to dependent type name 'RemoveCV<T>::Type'

libc/utils/MPFRWrapper/MPFRUtils.cpp

29–41

Yes. Having explicit enables avoids implicit conversions of the arguments. Implicit conversions can potentially lead of loss of precision. Added a comment saying the same.

43–44

Again, are you suggesting to put a static_assert inside the function body? If yes, it feels a bit odd to error-out with a custom message rather than give a normal compiler message for a constructor.

libc/utils/MPFRWrapper/MPFRUtils.h

75–77

Done, but I assumed you meant we should instead put a static_assert inside the function body? I think that makes sense. But, see my comment at the other place. I don't have any strong preference, but I do see that in this case the static_assert inside the function body is much nicer.

[abrachet]

LGTM

libc/utils/CPP/TypeTraits.h

57

My mistake, I thought it wasn't necessary because the right hand side of a using can only be a type.

libc/utils/MPFRWrapper/MPFRUtils.cpp

43–44

Sounds good to keep it like this then

libc/test/src/math/cosf_test.cpp

 
 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))
       continue;
-    EXPECT_TRUE(mpfr::equalsCos(x, __llvm_libc::cosf(x), tolerance));
+    ASSERT_MPFR_MATCH(mpfr::OP_Cos, x, __llvm_libc::cosf(x), tolerance);
   }
 }
 
 // For small values, cos(x) is 1.
 TEST(CosfTest, SmallValues) {
   float x = as_float(0x17800000);
   float result = __llvm_libc::cosf(x);
-  EXPECT_TRUE(mpfr::equalsCos(x, result, tolerance));
+  EXPECT_MPFR_MATCH(mpfr::OP_Cos, x, result, tolerance);
   EXPECT_EQ(FloatBits::One, as_uint32_bits(result));
 
-  x = as_float(0x00400000);
+  x = as_float(0x0040000);
   result = __llvm_libc::cosf(x);
-  EXPECT_TRUE(mpfr::equalsCos(x, result, tolerance));
+  EXPECT_MPFR_MATCH(mpfr::OP_Cos, x, result, tolerance);
   EXPECT_EQ(FloatBits::One, as_uint32_bits(result));
 }
 
 // SDCOMP-26094: check cosf in the cases for which the range reducer
 // returns values furthest beyond its nominal upper bound of pi/4.
 TEST(CosfTest, SDCOMP_26094) {
   for (uint32_t v : sdcomp26094Values) {
     float x = as_float(v);
-    EXPECT_TRUE(mpfr::equalsCos(x, __llvm_libc::cosf(x), tolerance));
+    ASSERT_MPFR_MATCH(mpfr::OP_Cos, x, __llvm_libc::cosf(x), tolerance);
   }
 }

libc/test/src/math/sincosf_test.cpp

TEST(SinCosfTest, InFloatRange) {
   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))
       continue;
 
     float sin, cos;
     __llvm_libc::sincosf(x, &sin, &cos);
-    EXPECT_TRUE(mpfr::equalsCos(x, cos, tolerance));
-    EXPECT_TRUE(mpfr::equalsSin(x, sin, tolerance));
+    ASSERT_MPFR_MATCH(mpfr::OP_Cos, x, cos, tolerance);
+    ASSERT_MPFR_MATCH(mpfr::OP_Sin, x, sin, tolerance);
   }
 }
 
 // For small values, cos(x) is 1 and sin(x) is x.
 TEST(SinCosfTest, SmallValues) {
   uint32_t bits = 0x17800000;
   float x = as_float(bits);
   float result_cos, result_sin;
   __llvm_libc::sincosf(x, &result_sin, &result_cos);
-  EXPECT_TRUE(mpfr::equalsCos(x, result_cos, tolerance));
-  EXPECT_TRUE(mpfr::equalsSin(x, result_sin, tolerance));
+  EXPECT_MPFR_MATCH(mpfr::OP_Cos, x, result_cos, tolerance);
+  EXPECT_MPFR_MATCH(mpfr::OP_Sin, x, result_sin, tolerance);
   EXPECT_EQ(FloatBits::One, as_uint32_bits(result_cos));
   EXPECT_EQ(bits, as_uint32_bits(result_sin));
 
   bits = 0x00400000;
   x = as_float(bits);
   __llvm_libc::sincosf(x, &result_sin, &result_cos);
-  EXPECT_TRUE(mpfr::equalsCos(x, result_cos, tolerance));
-  EXPECT_TRUE(mpfr::equalsSin(x, result_sin, tolerance));
+  EXPECT_MPFR_MATCH(mpfr::OP_Cos, x, result_cos, tolerance);
+  EXPECT_MPFR_MATCH(mpfr::OP_Sin, x, result_sin, tolerance);
   EXPECT_EQ(FloatBits::One, as_uint32_bits(result_cos));
   EXPECT_EQ(bits, as_uint32_bits(result_sin));
 }
 
 // SDCOMP-26094: check sinf in the cases for which the range reducer
 // returns values furthest beyond its nominal upper bound of pi/4.
 TEST(SinCosfTest, SDCOMP_26094) {
   for (uint32_t v : sdcomp26094Values) {
     float x = as_float(v);
     float sin, cos;
     __llvm_libc::sincosf(x, &sin, &cos);
-    EXPECT_TRUE(mpfr::equalsCos(x, cos, tolerance));
-    EXPECT_TRUE(mpfr::equalsSin(x, sin, tolerance));
+    EXPECT_MPFR_MATCH(mpfr::OP_Cos, x, cos, tolerance);
+    EXPECT_MPFR_MATCH(mpfr::OP_Sin, x, sin, tolerance);
   }
 }

libc/test/src/math/sinf_test.cpp

 
 TEST(SinfTest, 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))
       continue;
-    EXPECT_TRUE(mpfr::equalsSin(x, __llvm_libc::sinf(x), tolerance));
+    ASSERT_MPFR_MATCH(mpfr::OP_Sin, x, __llvm_libc::sinf(x), tolerance);
   }
 }
 
 TEST(SinfTest, SpecificBitPatterns) {
   float x = as_float(0xc70d39a1);
-  EXPECT_TRUE(mpfr::equalsSin(x, __llvm_libc::sinf(x), tolerance));
+  EXPECT_MPFR_MATCH(mpfr::OP_Sin, x, __llvm_libc::sinf(x), tolerance);
 }
 
 // For small values, sin(x) is x.
 TEST(SinfTest, SmallValues) {
   uint32_t bits = 0x17800000;
   float x = as_float(bits);
   float result = __llvm_libc::sinf(x);
-  EXPECT_TRUE(mpfr::equalsSin(x, result, tolerance));
+  EXPECT_MPFR_MATCH(mpfr::OP_Sin, x, result, tolerance);
   EXPECT_EQ(bits, as_uint32_bits(result));
 
   bits = 0x00400000;
   x = as_float(bits);
   result = __llvm_libc::sinf(x);
-  EXPECT_TRUE(mpfr::equalsSin(x, result, tolerance));
+  EXPECT_MPFR_MATCH(mpfr::OP_Sin, x, result, tolerance);
   EXPECT_EQ(bits, as_uint32_bits(result));
 }
 
 // SDCOMP-26094: check sinf in the cases for which the range reducer
 // returns values furthest beyond its nominal upper bound of pi/4.
 TEST(SinfTest, SDCOMP_26094) {
   for (uint32_t v : sdcomp26094Values) {
     float x = as_float(v);
-    EXPECT_TRUE(mpfr::equalsSin(x, __llvm_libc::sinf(x), tolerance));
+    EXPECT_MPFR_MATCH(mpfr::OP_Sin, x, __llvm_libc::sinf(x), tolerance);
   }
 }

libc/utils/CPP/TypeTraits.h

 template <> struct IsIntegral<bool> : public TrueValue {};
 
 template <typename T> struct IsPointerType : public FalseValue {};
 template <typename T> struct IsPointerType<T *> : public TrueValue {};
 
 template <typename T1, typename T2> struct IsSame : public FalseValue {};
 template <typename T> struct IsSame<T, T> : public TrueValue {};
 
+template <typename T> struct TypeIdentity { typedef T Type; };
+
+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 IsFloatingPointType {
+  static constexpr bool Value = IsSame<float, RemoveCVType<Type>>::Value ||
+                                IsSame<double, RemoveCVType<Type>>::Value ||
+                                IsSame<long double, RemoveCVType<Type>>::Value;
+};
+
 } // namespace cpp
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_UTILS_CPP_TYPETRAITS_H

libc/utils/MPFRWrapper/CMakeLists.txt

 try_compile(
   LIBC_TESTS_CAN_USE_MPFR
   ${CMAKE_CURRENT_BINARY_DIR}
   SOURCES
     ${CMAKE_CURRENT_SOURCE_DIR}/check_mpfr.cpp
   LINK_LIBRARIES
     -lmpfr -lgmp
 )
 
 if(LIBC_TESTS_CAN_USE_MPFR)
   add_library(libcMPFRWrapper
     MPFRUtils.cpp
     MPFRUtils.h
   )
-  target_link_libraries(libcMPFRWrapper -lmpfr -lgmp)
+  add_dependencies(libcMPFRWrapper libc.utils.CPP.standalone_cpp LibcUnitTest LLVMSupport)
+  target_link_libraries(libcMPFRWrapper -lmpfr -lgmp LibcUnitTest LLVMSupport)
 else()
   message(WARNING "Math tests using MPFR will be skipped.")
 endif()

libc/utils/MPFRWrapper/MPFRUtils.h

 //===-- MPFRUtils.h ---------------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_UTILS_TESTUTILS_MPFRUTILS_H
 #define LLVM_LIBC_UTILS_TESTUTILS_MPFRUTILS_H
 
+#include "utils/CPP/TypeTraits.h"
+#include "utils/UnitTest/Test.h"
+
 #include <stdint.h>
 
 namespace __llvm_libc {
 namespace testing {
 namespace mpfr {
 
 struct Tolerance {
   // Number of bits used to represent the fractional
struct Tolerance {
   unsigned int width; // Number of valid LSB bits in |value|.
 
   // The bits in the tolerance value. The tolerance value will be
   // sum(bits[width - i] * 2 ^ (- basePrecision - i)) for |i| in
   // range [1, width].
   uint32_t bits;
 };
 
-// Return true if |libcOutput| is within the tolerance |t| of the cos(x)
-// value as evaluated by MPFR.
-bool equalsCos(float x, float libcOutput, const Tolerance &t);
-
-// Return true if |libcOutput| is within the tolerance |t| of the sin(x)
-// value as evaluated by MPFR.
-bool equalsSin(float x, float libcOutput, const Tolerance &t);
+enum Operation {
+  OP_Cos,
+  OP_Sin,
+};
+
+namespace internal {
+
+template <typename T>
+bool compare(Operation op, T input, T libcOutput, const Tolerance &t);
+
+template <typename T> class MPFRMatcher : public testing::Matcher<T> {
+  static_assert(__llvm_libc::cpp::IsFloatingPointType<T>::Value,
+                "MPFRMatcher can only be used with floating point values.");
+
+  Operation operation;
+  T input;
+  Tolerance tolerance;
+  T matchValue;
+
+public:
+  MPFRMatcher(Operation op, T testInput, Tolerance &t)
+      : operation(op), input(testInput), tolerance(t) {}
+
+  bool match(T libcResult) {
+    matchValue = libcResult;
+    return internal::compare(operation, input, libcResult, tolerance);
+  }
+
+  void explainError(testutils::StreamWrapper &OS) override;
+};
+
+} // namespace internal
+
+template <typename T,
+          __llvm_libc::cpp::EnableIfType<
+              __llvm_libc::cpp::IsFloatingPointType<T>::Value, int> = 0>
+internal::MPFRMatcher<T> getMPFRMatcher(Operation op, T input, Tolerance t) {
+  return internal::MPFRMatcher<T>(op, input, t);
+}
 
 } // namespace mpfr
 } // namespace testing
 } // namespace __llvm_libc
 
+#define EXPECT_MPFR_MATCH(op, input, matchValue, tolerance)                    \
+  EXPECT_THAT(matchValue, __llvm_libc::testing::mpfr::getMPFRMatcher(          \
+                              op, input, tolerance))
+
+#define ASSERT_MPFR_MATCH(op, input, matchValue, tolerance)                    \
+  ASSERT_THAT(matchValue, __llvm_libc::testing::mpfr::getMPFRMatcher(          \
+                              op, input, tolerance))
+
 #endif // LLVM_LIBC_UTILS_TESTUTILS_MPFRUTILS_H

libc/utils/MPFRWrapper/MPFRUtils.cpp

 //===-- Utils which wrap MPFR ---------------------------------------------===//
 //
 // 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 "MPFRUtils.h"
 
-#include <iostream>
+#include "llvm/Support/raw_ostream.h"
+
 #include <mpfr.h>
 
 namespace __llvm_libc {
 namespace testing {
 namespace mpfr {
 
 class MPFRNumber {
   // A precision value which allows sufficiently large additional
   // precision even compared to double precision floating point values.
   static constexpr unsigned int mpfrPrecision = 96;
 
   mpfr_t value;
 
 public:
   MPFRNumber() { mpfr_init2(value, mpfrPrecision); }
 
-  explicit MPFRNumber(float x) {
+  template <typename XType,
+            cpp::EnableIfType<cpp::IsSame<float, XType>::Value, int> = 0>
+  explicit MPFRNumber(XType x) {
     mpfr_init2(value, mpfrPrecision);
     mpfr_set_flt(value, x, MPFR_RNDN);
   }
 
+  template <typename XType,
+            cpp::EnableIfType<cpp::IsSame<double, XType>::Value, int> = 0>
+  explicit MPFRNumber(XType x) {
+    mpfr_init2(value, mpfrPrecision);
+    mpfr_set_d(value, x, MPFR_RNDN);
+  }
+
+  template <typename XType,
+            cpp::EnableIfType<cpp::IsFloatingPointType<XType>::Value, int> = 0>
+  MPFRNumber(Operation op, XType rawValue) {
+    mpfr_init2(value, mpfrPrecision);
+    MPFRNumber mpfrInput(rawValue);
+    switch (op) {
+    case OP_Cos:
+      mpfr_cos(value, mpfrInput.value, MPFR_RNDN);
+      break;
+    case OP_Sin:
+      mpfr_sin(value, mpfrInput.value, MPFR_RNDN);
+      break;
+    }
+  }
+
   MPFRNumber(const MPFRNumber &other) {
     mpfr_set(value, other.value, MPFR_RNDN);
   }
 
   ~MPFRNumber() { mpfr_clear(value); }
 
   // Returns true if |other| is within the tolerance value |t| of this
   // number.
bool isEqual(const MPFRNumber &other, const Tolerance &t) {
     if (mpfr_cmp(value, other.value) >= 0)
       mpfr_sub(difference.value, value, other.value, MPFR_RNDN);
     else
       mpfr_sub(difference.value, other.value, value, MPFR_RNDN);
 
     return mpfr_lessequal_p(difference.value, tolerance.value);
   }
 
+  std::string str() const {
+    // 200 bytes should be more than sufficient to hold a 100-digit number
+    // plus additional bytes for the decimal point, '-' sign etc.
+    constexpr size_t printBufSize = 200;
+    char buffer[printBufSize];
+    mpfr_snprintf(buffer, printBufSize, "%100.50Rf", value);
+    llvm::StringRef ref(buffer);
+    ref = ref.trim();
+    return ref.str();
+  }
+
   // These functions are useful for debugging.
   float asFloat() const { return mpfr_get_flt(value, MPFR_RNDN); }
   double asDouble() const { return mpfr_get_d(value, MPFR_RNDN); }
   void dump(const char *msg) const { mpfr_printf("%s%.128Rf\n", msg, value); }
+};
 
-public:
-  static MPFRNumber cos(float x) {
-    MPFRNumber result;
-    MPFRNumber mpfrX(x);
-    mpfr_cos(result.value, mpfrX.value, MPFR_RNDN);
-    return result;
-  }
+namespace internal {
 
-  static MPFRNumber sin(float x) {
-    MPFRNumber result;
-    MPFRNumber mpfrX(x);
-    mpfr_sin(result.value, mpfrX.value, MPFR_RNDN);
-    return result;
-  }
+template <typename T>
+void MPFRMatcher<T>::explainError(testutils::StreamWrapper &OS) {
+  MPFRNumber mpfrResult(operation, input);
+  MPFRNumber mpfrInput(input);
+  MPFRNumber mpfrMatchValue(matchValue);
+  OS << "Match value not within tolerance value of MPFR result:\n"
+     << "Operation input: " << mpfrInput.str() << '\n'
+     << "    Match value: " << mpfrMatchValue.str() << '\n'
+     << "    MPFR result: " << mpfrResult.str() << '\n';
+}
+
+template void MPFRMatcher<float>::explainError(testutils::StreamWrapper &);
+template void MPFRMatcher<double>::explainError(testutils::StreamWrapper &);
+
+template <typename T>
+bool compare(Operation op, T input, T libcResult, const Tolerance &t) {
+  MPFRNumber mpfrResult(op, input);
+  MPFRNumber mpfrInput(input);
+  MPFRNumber mpfrLibcResult(libcResult);
+  return mpfrResult.isEqual(mpfrLibcResult, t);
 };
 
-bool equalsCos(float input, float libcOutput, const Tolerance &t) {
-  MPFRNumber mpfrResult = MPFRNumber::cos(input);
-  MPFRNumber libcResult(libcOutput);
-  return mpfrResult.isEqual(libcResult, t);
-}
+template bool compare<float>(Operation, float, float, const Tolerance &);
+template bool compare<double>(Operation, double, double, const Tolerance &);
 
-bool equalsSin(float input, float libcOutput, const Tolerance &t) {
-  MPFRNumber mpfrResult = MPFRNumber::sin(input);
-  MPFRNumber libcResult(libcOutput);
-  return mpfrResult.isEqual(libcResult, t);
-}
+} // namespace internal
 
 } // namespace mpfr
 } // namespace testing
 } // namespace __llvm_libc

libc/utils/testutils/StreamWrapper.cpp

 //===-- StreamWrapper.cpp -------------------------------------------------===//
 //
 // 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 "StreamWrapper.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <memory>
+#include <string>
 
 namespace __llvm_libc {
 namespace testutils {
 
 StreamWrapper outs() { return {std::addressof(llvm::outs())}; }
 
 template <typename T> StreamWrapper &StreamWrapper::operator<<(T t) {
   assert(OS);

 template StreamWrapper &
     StreamWrapper::operator<<<unsigned short>(unsigned short t);
 template StreamWrapper &StreamWrapper::operator<<<unsigned int>(unsigned int t);
 template StreamWrapper &
     StreamWrapper::operator<<<unsigned long>(unsigned long t);
 template StreamWrapper &
     StreamWrapper::operator<<<unsigned long long>(unsigned long long t);
 template StreamWrapper &StreamWrapper::operator<<<bool>(bool t);
+template StreamWrapper &StreamWrapper::operator<<<std::string>(std::string t);
 
 } // namespace testutils
 } // namespace __llvm_libc