Fix random number generation and floating point comparison in matrix-types-spec.cpp

[jsji]

Summary

The testsuite is failing on PowerPC buildbots due to https://reviews.llvm.org/D72770.
Looks like we are using wrong type generator for double.

In file included from ...SingleSource/UnitTests/matrix-types-spec.cpp:8:
In file included from /usr/lib/gcc/powerpc64le-linux-gnu/7.3.0/../../../../include/c++/7.3.0/random:49:
In file included from /usr/lib/gcc/powerpc64le-linux-gnu/7.3.0/../../../../include/c++/7.3.0/bits/random.h:35:
/usr/lib/gcc/powerpc64le-linux-gnu/7.3.0/../../../../include/c++/7.3.0/bits/uniform_int_dist.h:63:7: error: static_assert failed due to requirement 'std::is_integral<double>::value' "template argument must be an integral type"

static_assert(std::is_integral<_IntType>::value,
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

...SingleSource/UnitTests/matrix-types-spec.cpp:38:41: note: in instantiation of template class 'std::uniform_int_distribution<double>' requested here

std::uniform_int_distribution<double> distribution(-10.0, 10.0);
                                      ^

...SingleSource/UnitTests/matrix-types-spec.cpp:39:29: error: no member named 'bind' in namespace 'std'; did you mean 'find'?

auto random_double = std::bind(distribution, generator);
                     ~~~~~^~~~
                          find

[fhahn]

Thanks for taking a look at the issue!

SingleSource/UnitTests/matrix-types-spec.cpp

90

I think we need 2 version of initRandom, one for floating point and one for integral element types, like in D83692, otherwise we might run into the same issue for integer element types.

For example

Floating point specialization:

template <typename ElementTy, 
          typename std::enable_if_t<std::is_floating_point<ElementTy>::value,
                                    int> = 0>
 void initRandom(ElementTy *A, unsigned Rows, unsigned Cols) {
  std::default_random_engine generator;
  std::uniform_real_distribution<ElementTy> distribution(-10.0, 10.0);
  ....
}

Integer specialization

template <
    typename ElementTy,
    typename std::enable_if_t<std::is_integral<ElementTy>::value, int> = 0>
 void initRandom(ElementTy *A, unsigned Rows, unsigned Cols) {
  std::default_random_engine generator;
  std::uniform_int_distribution<ElementTy> distribution(-10, 10);
  ....
}

[jsji]

• Add int specification version
  
  This will pass compilation, but runtime will still fail on PowerPC.
  
  The problem is we are using EXPECT_MATRIX_EQ to do binary comparision of floating point values before and afer multiplication. And there might be small differences but within tolerance allowance.
  
  I think we should output the data, fpcmp-target will handle the compare and tolerance.

[fhahn]

In D83910#2155803, @jsji wrote:

• Add int specification version
  
  This will pass compilation, but runtime will still fail on PowerPC.
  
  The problem is we are using EXPECT_MATRIX_EQ to do binary comparision of floating point values before and afer multiplication. And there might be small differences but within tolerance allowance.
  
  I think we should output the data, fpcmp-target will handle the compare and tolerance.

Oh right I see. I thought that the low input range would make that unlikely, but it would probably be best to check that the difference of both values is below a certain epsilon value. I think this could easily be done in the EXPECT_MATRIX_EQ macro, without needing to dump the matrixes and generate a large comparison file. I think it is an advantage to have a self-checking binary here, without the need of updating/maintaining a comparison file. What do you think?

[jsji]

• Use fpcmp to do floating point comparision

I copied the comparision code from fpcmp.c.

[jsji]

• Rename the function name

[jsji]

With this, the case is passing on PowerPC. @fhahn Can you have a look and also try on your platform? Thanks.

[fhahn]

LGTM, thank you very much! Looks good on both X86 and AArch64. The commit message could potentially be a bit more descriptive, .e.g something like Fix random number generation in matrix-types-spec.cpp. I don't think you need to include [test-suite], as the code lives in the llvm-test-suite repo, so the tag would be redundant.

[jsji]

Thanks. Committed in https://github.com/llvm/llvm-test-suite/commit/4bdbe4ad9dd25b432d405d8a6ac6b7410da9b7a2.

SingleSource/UnitTests/matrix-types-spec.cpp

 // Runtime tests to ensure the Matrix Type extension implementation matches the
 // specification.
 
 // Only build the tests if the proper extensions is available.
 #if __has_extension(matrix_types)
 
 #include <iostream>
 #include <random>
 #include <stdlib.h>
 
-#define EXPECT_MATRIX_EQ(A, B, R, C)                                           \
-  do {                                                                         \
-    for (unsigned r = 0; r < R; r++)                                           \
-      for (unsigned c = 0; c < C; c++)                                         \
-        if (A[r + c * R] != B[r + c * R]) {                                    \
-          std::cerr << "mismatch at " << r << ":" << c << "\n";                \
-          exit(1);                                                             \
-        }                                                                      \
-  } while (false)
+#define ABSTOL 0.000001
+#define RELTOL 0.00001
+bool fpcmp(double V1, double V2, double AbsTolerance, double RelTolerance) {
+  // Check to see if these are inside the absolute tolerance
+  if (AbsTolerance < fabs(V1 - V2)) {
+    // Nope, check the relative tolerance...
+    double Diff;
+    if (V2)
+      Diff = fabs(V1 / V2 - 1.0);
+    else if (V1)
+      Diff = fabs(V2 / V1 - 1.0);
+    else
+      Diff = 0; // Both zero.
+    if (Diff > RelTolerance) {
+      return true;
+    }
+  }
+  return false;
+}
+
+template <typename ElementTy, typename std::enable_if_t<
+                                  std::is_integral<ElementTy>::value, int> = 0>
+void expectMatrixEQ(ElementTy *A, ElementTy *B, unsigned R, unsigned C) {
+  do {
+    for (unsigned r = 0; r < R; r++)
+      for (unsigned c = 0; c < C; c++)
+        if (A[r + c * R] != B[r + c * R]) {
+          std::cerr << "mismatch at " << r << ":" << c << "\n";
+          exit(1);
+        }
+  } while (false);
+}
+
+template <typename ElementTy,
+          typename std::enable_if_t<std::is_floating_point<ElementTy>::value,
+                                    int> = 0>
+void expectMatrixEQ(ElementTy *A, ElementTy *B, unsigned R,
+                      unsigned C) {
+  do {
+    for (unsigned r = 0; r < R; r++)
+      for (unsigned c = 0; c < C; c++)
+        if (fpcmp(A[r + c * R], B[r + c * R], ABSTOL, RELTOL)) {
+          std::cerr << "mismatch at " << r << ":" << c << "\n";
+          exit(1);
+        }
+  } while (false);
+}
+
 
 template <typename EltTy>
 void zeroMatrix(EltTy *M, unsigned Rows, unsigned Cols) {
   for (unsigned r = 0; r < Rows; r++)
     for (unsigned c = 0; c < Cols; c++)
       M[r + c * Rows] = 0;
 }
 
 template <typename EltTy> void print(EltTy *X, unsigned Rows, unsigned Cols) {
   for (int r = 0; r < Rows; r++) {
     for (int c = 0; c < Cols; c++)
       std::cout << X[r + c * Rows] << ", ";
     std::cout << "\n";
   }
 }
 
-template <typename Ty> void initRandom(Ty *A, unsigned Rows, unsigned Cols) {
+template <typename ElementTy,
+          typename std::enable_if_t<std::is_floating_point<ElementTy>::value,
+                                    int> = 0>
+void initRandom(ElementTy *A, unsigned Rows, unsigned Cols) {
+  std::default_random_engine generator;
+  std::uniform_real_distribution<ElementTy> distribution(-10.0, 10.0);
+
+  for (unsigned i = 0; i < Rows * Cols; i++)
+    A[i] = distribution(generator);
+}
+
+template <typename ElementTy, typename std::enable_if_t<
+                                  std::is_integral<ElementTy>::value, int> = 0>
+void initRandom(ElementTy *A, unsigned Rows, unsigned Cols) {
   std::default_random_engine generator;
-  std::uniform_int_distribution<double> distribution(-10.0, 10.0);
-  auto random_double = std::bind(distribution, generator);
+  std::uniform_int_distribution<ElementTy> distribution(-10, 10);
 
   for (unsigned i = 0; i < Rows * Cols; i++)
-    A[i] = random_double();
+    A[i] = distribution(generator);
 }
 
 template <typename EltTy, unsigned R, unsigned C>
 void transposeBuiltin(EltTy *ResPtr, EltTy *SrcPtr) {
   auto M = __builtin_matrix_column_major_load(SrcPtr, R, C, R);
   __builtin_matrix_column_major_store(__builtin_matrix_transpose(M), ResPtr, C);
 }
 
template <typename EltTy, unsigned R0, unsigned C0> void testTranspose() {
   zeroMatrix(ResBase, C0, R0);
   zeroMatrix(ResSpec, C0, R0);
   zeroMatrix(ResBuiltin, C0, R0);
 
   transposeBase(ResBase, X, R0, C0);
   transposeSpec<EltTy, R0, C0>(ResSpec, X);
   transposeBuiltin<EltTy, R0, C0>(ResBuiltin, X);
 
-  EXPECT_MATRIX_EQ(ResBase, ResBuiltin, R0, C0);
-  EXPECT_MATRIX_EQ(ResBase, ResSpec, C0, R0);
+  expectMatrixEQ(ResBase, ResBuiltin, R0, C0);
+  expectMatrixEQ(ResBase, ResSpec, C0, R0);
 }
 
 template <typename EltTy, unsigned R0, unsigned C0, unsigned C1>
 void multiplyBuiltin(EltTy *Res, EltTy *Matrix1, EltTy *Matrix2) {
   using res_t = EltTy __attribute__((matrix_type(R0, C1)));
 
   auto A = __builtin_matrix_column_major_load(Matrix1, R0, C0, R0);
   auto B = __builtin_matrix_column_major_load(Matrix2, C0, C1, C0);
void testMultiply() {
   zeroMatrix(ResBase, R0, C1);
   zeroMatrix(ResSpec, R0, C1);
   zeroMatrix(ResBuiltin, R0, C1);
 
   multiplyBase<EltTy, R0, C0, C1>(ResBase, X, Y);
   multiplySpec<EltTy, R0, C0, C1>(ResSpec, X, Y);
   multiplyBuiltin<EltTy, R0, C0, C1>(ResBuiltin, X, Y);
 
-  EXPECT_MATRIX_EQ(ResSpec, ResBuiltin, R0, C1);
-  EXPECT_MATRIX_EQ(ResBase, ResBuiltin, R0, C1);
-  EXPECT_MATRIX_EQ(ResBase, ResSpec, R0, C1);
+  expectMatrixEQ(ResSpec, ResBuiltin, R0, C1);
+  expectMatrixEQ(ResBase, ResBuiltin, R0, C1);
+  expectMatrixEQ(ResBase, ResSpec, R0, C1);
 }
 
 int main(void) {
   testTranspose<double, 3, 3>();
   testTranspose<double, 3, 10>();
   testTranspose<double, 4, 3>();
   testTranspose<float, 31, 17>();
   testTranspose<unsigned, 8, 7>();