[mlir][sparse] minor cleanup of merger unit test

[aartbik]

Summary

Removed some of the warning supression needed for
the multi-arg macro logic by making number of arguments
the same everywhere. Also removes some verbose comments
and obvious TODOs.

[wrengr]

mlir/unittests/Dialect/SparseTensor/MergerTest.cpp

496

Why not call this UNUSED instead? (ditto for the other macros below)

[aartbik]

mlir/unittests/Dialect/SparseTensor/MergerTest.cpp

496

sgtm

[aartbik]

UNUSED instead of DROP_EXTRA

[wrengr]

mlir/unittests/Dialect/SparseTensor/MergerTest.cpp

520

Since the only place that uses the extra argument to FOREVERY_COMMON_{DISJ,CONJ}_BINOP is the definition of the macros FOREVERY_PAIR_OF_COMMON_{DISJ,CONJ}_DISJ_BINOP, I think it'd be a bit cleaner to do something like the following:

define FOREVERY_COMMON_X_BINOP_EXTRA(TEST, EXTRA) ...
define FOREVERY_COMMON_X_BINOP(TEST) \
    FOREVERY_COMMON_X_BINOP_EXTRA(TEST, "")

and then use the former for defining the other macros, and use the latter throughout the file. That way you don't need to explicitly pass the empty string every time

[wrengr]

lgtm, once you do the FOREVERY_COMMON_X_BINOP{_EXTRA,} split to help DRY the uses

[aartbik]

less strings, more macros

mlir/unittests/Dialect/SparseTensor/MergerTest.cpp

 #include "mlir/Dialect/SparseTensor/Utils/Merger.h"
 #include "llvm/Support/Compiler.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+
 #include <memory>
 
 using namespace mlir;
 using namespace mlir::sparse_tensor;
 
-// Silence 'warning C4002: 'too many arguments for function-liked macro
-//                          invocation'
-// as MSVC handles ##__VA_ARGS__ differently as gcc/clang
-
-#if defined(_MSC_VER) && !defined(__clang__)
-#pragma warning(push)
-#pragma warning(disable : 4002)
-#endif
-
 namespace {
 
 ///
 /// Defines macros to iterate binary and the combination of binary operations.
 ///
 
 #define FOREVERY_BINOP(DO)                                                     \
   DO(mulf, TensorExp::Kind::kMulF)                                             \
   DO(mulc, TensorExp::Kind::kMulC)                                             \
   DO(muli, TensorExp::Kind::kMulI)                                             \
   DO(addf, TensorExp::Kind::kAddF)                                             \
   DO(addc, TensorExp::Kind::kAddC)                                             \
   DO(addi, TensorExp::Kind::kAddI)                                             \
   DO(subf, TensorExp::Kind::kSubF)                                             \
   DO(subc, TensorExp::Kind::kSubC)                                             \
   DO(subi, TensorExp::Kind::kSubI)                                             \
   DO(andi, TensorExp::Kind::kAndI)                                             \
   DO(xori, TensorExp::Kind::kXorI)                                             \
   DO(ori, TensorExp::Kind::kOrI)                                               \
   DO(cmpf, TensorExp::Kind::kCmpF)                                             \
   DO(cmpi, TensorExp::Kind::kCmpI)
 
-// TODO: Disjunctive binary operations that need special handling are not
-// included, e.g., Division are not tested (for now) as it need a constant
-// non-zero dividend.
-// ##__VA_ARGS__ handles cases when __VA_ARGS__ is empty.
-#define FOREVERY_COMMON_DISJ_BINOP(TEST, ...)                                  \
-  TEST(addf, ##__VA_ARGS__)                                                    \
-  TEST(addc, ##__VA_ARGS__)                                                    \
-  TEST(addi, ##__VA_ARGS__)                                                    \
-  TEST(xori, ##__VA_ARGS__)                                                    \
-  TEST(ori, ##__VA_ARGS__)
-
-// TODO: Conjunctive binary operations that need special handling are not
-// included, e.g., substraction yields a different pattern as it is mapped to
-// negate operation.
-#define FOREVERY_COMMON_CONJ_BINOP(TEST, ...)                                  \
-  TEST(mulf, ##__VA_ARGS__)                                                    \
-  TEST(mulc, ##__VA_ARGS__)                                                    \
-  TEST(muli, ##__VA_ARGS__)                                                    \
-  TEST(andi, ##__VA_ARGS__)
+#define FOREVERY_COMMON_DISJ_BINOP_EXTRA(TEST, EXTRA)                          \
+  TEST(addf, EXTRA)                                                            \
+  TEST(addc, EXTRA)                                                            \
+  TEST(addi, EXTRA)                                                            \
+  TEST(xori, EXTRA)                                                            \
+  TEST(ori, EXTRA)
+
+#define FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, EXTRA)                          \
+  TEST(mulf, EXTRA)                                                            \
+  TEST(mulc, EXTRA)                                                            \
+  TEST(muli, EXTRA)                                                            \
+  TEST(andi, EXTRA)
+
+#define FOREVERY_COMMON_DISJ_BINOP(TEST)                                       \
+  FOREVERY_COMMON_DISJ_BINOP_EXTRA(TEST, "")
+
+#define FOREVERY_COMMON_CONJ_BINOP(TEST)                                       \
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, "")
 
 #define FOREVERY_PAIR_OF_COMMON_CONJ_DISJ_BINOP(TEST)                          \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, addf)                                       \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, addc)                                       \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, addi)                                       \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, xori)                                       \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, ori)
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, addf)                                 \
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, addc)                                 \
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, addi)                                 \
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, xori)                                 \
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, ori)
 
 #define FOREVERY_PAIR_OF_COMMON_CONJ_CONJ_BINOP(TEST)                          \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, mulf)                                       \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, mulc)                                       \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, muli)                                       \
-  FOREVERY_COMMON_CONJ_BINOP(TEST, andi)
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, mulf)                                 \
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, mulc)                                 \
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, muli)                                 \
+  FOREVERY_COMMON_CONJ_BINOP_EXTRA(TEST, andi)
 
 #define FOREVERY_PAIR_OF_COMMON_DISJ_DISJ_BINOP(TEST)                          \
-  FOREVERY_COMMON_DISJ_BINOP(TEST, addf)                                       \
-  FOREVERY_COMMON_DISJ_BINOP(TEST, addc)                                       \
-  FOREVERY_COMMON_DISJ_BINOP(TEST, addi)                                       \
-  FOREVERY_COMMON_DISJ_BINOP(TEST, ori)                                        \
-  FOREVERY_COMMON_DISJ_BINOP(TEST, xori)
+  FOREVERY_COMMON_DISJ_BINOP_EXTRA(TEST, addf)                                 \
+  FOREVERY_COMMON_DISJ_BINOP_EXTRA(TEST, addc)                                 \
+  FOREVERY_COMMON_DISJ_BINOP_EXTRA(TEST, addi)                                 \
+  FOREVERY_COMMON_DISJ_BINOP_EXTRA(TEST, ori)                                  \
+  FOREVERY_COMMON_DISJ_BINOP_EXTRA(TEST, xori)
 
 ///
 /// Helper classes/functions for testing Merger.
 ///
 
 /// Simple recursive data structure used to match expressions in `Merger`.
 struct Pattern;
 /// Since the patterns we need are rather small and short-lived, we use
 /// `Pattern const&` for "pointers" to patterns, rather than using
 /// something more elaborate like `std::shared_ptr<Pattern> const&`.
-/// (But since we use a typedef rather than spelling it out everywhere,
-/// that's easy enough to swap out if we need something more elaborate
-/// in the future.)
 using PatternRef = const Pattern &;
 struct Pattern {
   struct Children {
     Children(PatternRef e0, PatternRef e1) : e0(e0), e1(e1) {}
     PatternRef e0;
     PatternRef e1;
   };
 
   TensorExp::Kind kind;
 
   union {
     /// Expressions representing tensors simply have a tensor number.
     TensorId tid;
 
     /// Tensor operations point to their children.
     Children children;
   };
 
   /// Constructors.
-  /// Rather than using these, please use the readable helper constructor
+  /// Rather than using these, please use the readable builder
   /// functions below to make tests more readable.
   Pattern() : kind(TensorExp::Kind::kSynZero) {}
   Pattern(TensorId tid) : kind(TensorExp::Kind::kTensor), tid(tid) {}
   Pattern(TensorExp::Kind kind, PatternRef e0, PatternRef e1)
       : kind(kind), children(e0, e1) {
     assert(kind >= TensorExp::Kind::kMulF);
   }
 };

 /// }
 /// and after optimization, the lattice points do not change (as there is no
 /// duplicated point and all input vectors are sparse vector).
 /// {
 ///   lat( i_00 i_01 / (tensor_0 + tensor_1) )
 ///   lat( i_00 / tensor_0 )
 ///   lat( i_01 / tensor_1 )
 /// }
-#define IMPL_MERGER_TEST_DISJ(OP)                                              \
+#define IMPL_MERGER_TEST_DISJ(OP, UNUSED)                                      \
   TEST_F(MergerTest3T1L, vector_##OP) {                                        \
     const auto e = OP##Expr(tensor(0), tensor(1));                             \
     const auto l0 = lid(0);                                                    \
     const auto t0 = tid(0);                                                    \
     const auto t1 = tid(1);                                                    \
     PatternRef p0 = tensorPattern(t0);                                         \
     PatternRef p1 = tensorPattern(t1);                                         \
     auto s = merger.buildLattices(e, l0);                                      \
                                                                                \
     expectNumLatPoints(s, 3);                                                  \
     expectLatPoint(s, 0, OP##Pattern(p0, p1),                                  \
                    loopsToBits({{l0, t0}, {l0, t1}}));                         \
     expectLatPointWithinRange(s, 1, 2, p0, loopsToBits({{l0, t0}}));           \
     expectLatPointWithinRange(s, 1, 2, p1, loopsToBits({{l0, t1}}));           \
                                                                                \
     s = merger.optimizeSet(s);                                                 \
     expectNumLatPoints(s, 3);                                                  \
     expectLatPoint(s, 0, OP##Pattern(p0, p1),                                  \
                    loopsToBits({{l0, t0}, {l0, t1}}), true);                   \
     expectLatPointWithinRange(s, 1, 2, p0, loopsToBits({{l0, t0}}), true);     \
     expectLatPointWithinRange(s, 1, 2, p1, loopsToBits({{l0, t1}}), true);     \
   }
 
 FOREVERY_COMMON_DISJ_BINOP(IMPL_MERGER_TEST_DISJ)
 
 #undef IMPL_MERGER_TEST_DISJ
 
 /// Vector multiplication (conjunction) of 2 vectors, i.e.;
 ///   a(i) = b(i) * c(i)
 /// which should form the single lattice point
 /// {
 ///   lat( i_00 i_01 / (tensor_0 * tensor_1) )
 /// }
-#define IMPL_MERGER_TEST_CONJ(OP)                                              \
+#define IMPL_MERGER_TEST_CONJ(OP, UNUSED)                                      \
   TEST_F(MergerTest3T1L, vector_##OP) {                                        \
     const auto e = OP##Expr(tensor(0), tensor(1));                             \
     const auto l0 = lid(0);                                                    \
     const auto t0 = tid(0);                                                    \
     const auto t1 = tid(1);                                                    \
     PatternRef p0 = tensorPattern(t0);                                         \
     PatternRef p1 = tensorPattern(t1);                                         \
     auto s = merger.buildLattices(e, l0);                                      \

 /// which should be optimized to
 /// {
 ///   lat( i_00 i_01 / (sparse_tensor_0 + dense_tensor_1) ) (not singleton)
 ///   lat( i_01 / dense_tensor_0 ) (no sparse dimension)
 /// }
 ///
 /// lat( i_00 / sparse_tensor_0 ) should be opted out as it only has dense diff
 /// with lat( i_00 i_01 / (sparse_tensor_0 + dense_tensor_1) ).
-#define IMPL_MERGER_TEST_OPTIMIZED_DISJ(OP)                                    \
+#define IMPL_MERGER_TEST_OPTIMIZED_DISJ(OP, UNUSED)                            \
   TEST_F(MergerTest3T1LD, vector_opted_##OP) {                                 \
     const auto e = OP##Expr(tensor(0), tensor(1));                             \
     const auto l0 = lid(0);                                                    \
     const auto t0 = tid(0);                                                    \
     const auto t1 = tid(1);                                                    \
     PatternRef p0 = tensorPattern(t0);                                         \
     PatternRef p1 = tensorPattern(t1);                                         \
     auto s = merger.buildLattices(e, l0);                                      \

 /// {
 ///   lat( i_00 i_01 / (sparse_tensor_0 * dense_tensor_1) )
 /// }
 /// it should be optimized to
 /// {
 ///   lat( i_00 / (sparse_tensor_0 * dense_tensor_1) )
 /// }
 /// since i_01 is a dense dimension.
-#define IMPL_MERGER_TEST_OPTIMIZED_CONJ(OP)                                    \
+#define IMPL_MERGER_TEST_OPTIMIZED_CONJ(OP, UNUSED)                            \
   TEST_F(MergerTest3T1LD, vector_opted_##OP) {                                 \
     const auto e = OP##Expr(tensor(0), tensor(1));                             \
     const auto l0 = lid(0);                                                    \
     const auto t0 = tid(0);                                                    \
     const auto t1 = tid(1);                                                    \
     PatternRef p0 = tensorPattern(t0);                                         \
     PatternRef p1 = tensorPattern(t1);                                         \
     auto s = merger.buildLattices(e, l0);                                      \
expectLatPointWithinRange(s, 1, 2, cmpiPattern(zero, p1),
                             loopsToBits({{l0, t1}}));
   s = merger.optimizeSet(s);
   expectLatPoint(s, 0, cmpiPattern(p0, p1), loopsToBits({{l0, t0}, {l0, t1}}));
   expectLatPointWithinRange(s, 1, 2, cmpiPattern(zero, p1),
                             loopsToBits({{l0, t1}}));
 }
 
 #undef IMPL_MERGER_TEST_OPTIMIZED_CONJ
-
-// TODO: mult-dim tests
-
-// restore warning status
-#if defined(_MSC_VER) && !defined(__clang__)
-#pragma warning(pop)
-#endif