[mlir] Detemplaize m_Op and RecursivePatternMatcher.

[chelini]

Summary

As Alex Zinenko pointed out, there is no need for the 'OpClass' template in the op_matcher and RecursivePatternMatcher classes. It suffices to store the unique name of the operation to be matched in the constructor and use it as the matching key.

[merge_guards_bot]

Unit tests: pass. 61250 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ftynse]

Thanks!

Please only change the format of the lines you touch in the commit, e.g. don't run clang-format -i but rather git clang-format (https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format)

mlir/include/mlir/IR/Matchers.h

54

This looks like unrelated change, and also wrong format.

129

We don't prefix StringRef with llvm, it's re-exported into the mlir namespace.

129

Please add explicit

131–133

return operationToMatch == op->getName().getStringRef()

137

If you intend to store StringRefs, please make it clear from the documentation that the caller of op_matcher is expected to keep the actual string this refers to alive at least as long as the op_matcher instance.

213

Can you just inline this function into the call place after removing the unneeded if?

214–216

Same as above

[chelini]

mlir/include/mlir/IR/Matchers.h

54

Hi Alex, thanks for the comments. I will get back to you with the fixes soon. But before I need a little clarification. I indeed run clang-format -i on the entire file but I used the clang-format file in the MLIR directory. The reason why the code has been formated this way is the second line in the clang-format file: here

Should I follow the standard dictates by this line or it is unwanted?

[rriddle]

mlir/lib/Dialect/StandardOps/Ops.cpp

200

This is broken. ConstantIndexOp is a "derived" operation from ConstantOp, and shares the same operation name. This revision makes the assumption that there are no "derived" operations(classes that override the base 'classof'), which is false:

https://github.com/llvm/llvm-project/blob/d67c4cc2eb4ddc450c886598b934c111e721ab0c/mlir/include/mlir/Dialect/StandardOps/Ops.h#L128

[rriddle]

mlir/include/mlir/IR/Matchers.h

54

You probably want to use 'git clang-format' instead.

[ftynse]

mlir/lib/Dialect/StandardOps/Ops.cpp

200

This is the same assumption PatternMatcher makes - https://github.com/llvm/llvm-project/blob/master/mlir/lib/IR/PatternMatch.cpp#L194, so we can get in trouble with OpRewritePattern<ConstantIndexOp> as well.

Do we have a lot of "derived" operations? If we truly want to support them within the pattern matching infra, we need some way of storing the pointer to classof. I can see the current approach (which doesn't scale if we want to match less trivial combinations of ops, such as permutations, since we would have to implement combinators as templates, which is arguably unreadable), a CRTP-based approach and a func_ref based approach. Any preference @rriddle, @nicolasvasilache ?

[lebedev.ri]

Passing-by remark:
i think the patch description only says what this patch does,
but not why, and does not spell out *why* this change is needed,

[nicolasvasilache]

We are now starting to introduce extra state and a parallel data structure to

mlir/include/mlir/IR/Matchers.h

 // tree-based pattern matching over MLIR. This mechanism is inspired by LLVM's
 // include/llvm/IR/PatternMatch.h.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_MATCHERS_H
 #define MLIR_MATCHERS_H
 
 #include "mlir/IR/OpDefinition.h"
 #include "mlir/IR/StandardTypes.h"
 
 namespace mlir {
 
 namespace detail {
 
 /// The matcher that matches a certain kind of Attribute and binds the value
 /// inside the Attribute.
if (auto intAttr = attr.dyn_cast<AttrClass>()) {
       return true;
     }
     return false;
   }
 };
 
 /// The matcher that matches a constant foldable operation that has no side
 /// effect, no operands and produces a single result.
-template <typename AttrT> struct constant_op_binder {
+template <typename AttrT>
+struct constant_op_binder {
   AttrT *bind_value;
 
   /// Creates a matcher instance that binds the constant attribute value to
   /// bind_value if match succeeds.
   constant_op_binder(AttrT *bind_value) : bind_value(bind_value) {}
 
   bool match(Operation *op) {
     if (op->getNumOperands() > 0 || op->getNumResults() != 1)
if (type.isa<VectorType>() || type.isa<RankedTensorType>()) {
       }
     }
     return false;
   }
 };
 
 /// The matcher that matches a given target constant scalar / vector splat /
 /// tensor splat integer value.
-template <int64_t TargetValue> struct constant_int_value_matcher {
+template <int64_t TargetValue>
+struct constant_int_value_matcher {
   bool match(Operation *op) {
     APInt value;
     return constant_int_op_binder(&value).match(op) && TargetValue == value;
   }
 };
 
 /// The matcher that matches anything except the given target constant scalar /
 /// vector splat / tensor splat integer value.
-template <int64_t TargetNotValue> struct constant_int_not_value_matcher {
+template <int64_t TargetNotValue>
+struct constant_int_not_value_matcher {
   bool match(Operation *op) {
     APInt value;
     return constant_int_op_binder(&value).match(op) && TargetNotValue != value;
   }
 };
 
 /// The matcher that matches a certain kind of op.
-template <typename OpClass> struct op_matcher {
-  bool match(Operation *op) { return isa<OpClass>(op); }
+class op_matcher {
+public:
+  op_matcher() = delete;
+  op_matcher(llvm::StringRef operation) : operationToMatch(operation){};
+  bool match(Operation *op) {
+    if (operationToMatch == op->getName().getStringRef())
+      return true;
+    return false;
+  }
+
+private:
+  llvm::StringRef operationToMatch;
 };
 
 /// Trait to check whether T provides a 'match' method with type
 /// `OperationOrValue`.
 template <typename T, typename OperationOrValue>
 using has_operation_or_value_matcher_t =
     decltype(std::declval<T>().match(std::declval<OperationOrValue>()));
 

 
 template <typename... Tys, typename CallbackT>
 constexpr void enumerate(std::tuple<Tys...> &tuple, CallbackT &&callback) {
   detail::enumerateImpl(tuple, std::forward<CallbackT>(callback),
                         std::make_index_sequence<sizeof...(Tys)>{});
 }
 
 /// RecursivePatternMatcher that composes.
-template <typename OpType, typename... OperandMatchers>
-struct RecursivePatternMatcher {
-  RecursivePatternMatcher(OperandMatchers... matchers)
-      : operandMatchers(matchers...) {}
+template <typename... OperandMatchers>
+class RecursivePatternMatcher {
+public:
+  RecursivePatternMatcher(llvm::StringRef operation,
+                          OperandMatchers... matchers)
+      : operationToMatch(operation), operandMatchers(matchers...) {}
   bool match(Operation *op) {
-    if (!isa<OpType>(op) || op->getNumOperands() != sizeof...(OperandMatchers))
+    if (!areSame(op) || op->getNumOperands() != sizeof...(OperandMatchers))
       return false;
     bool res = true;
     enumerate(operandMatchers, [&](size_t index, auto &matcher) {
       res &= matchOperandOrValueAtIndex(op, index, matcher);
     });
     return res;
   }
+
+private:
+  llvm::StringRef operationToMatch;
   std::tuple<OperandMatchers...> operandMatchers;
+  bool areSame(Operation *op) {
+    if (operationToMatch == op->getName().getStringRef())
+      return true;
+    return false;
+  }
 };
 
 } // end namespace detail
 
 /// Matches a value from a constant foldable operation and writes the value to
 /// bind_value.
 template <typename AttrT>
 inline detail::constant_op_binder<AttrT> m_Constant(AttrT *bind_value) {
   return detail::constant_op_binder<AttrT>(bind_value);
 }
 
 /// Matches a constant scalar / vector splat / tensor splat integer one.
 inline detail::constant_int_value_matcher<1> m_One() {
   return detail::constant_int_value_matcher<1>();
 }
 
 /// Matches the given OpClass.
-template <typename OpClass> inline detail::op_matcher<OpClass> m_Op() {
-  return detail::op_matcher<OpClass>();
+template <typename OpClass>
+inline detail::op_matcher m_Op() {
+  return detail::op_matcher(OpClass::getOperationName());
 }
 
 /// Matches a constant scalar / vector splat / tensor splat integer zero.
 inline detail::constant_int_value_matcher<0> m_Zero() {
   return detail::constant_int_value_matcher<0>();
 }
 
 /// Matches a constant scalar / vector splat / tensor splat integer that is any

 /// writes the integer value to bind_value.
 inline detail::constant_int_op_binder
 m_ConstantInt(IntegerAttr::ValueType *bind_value) {
   return detail::constant_int_op_binder(bind_value);
 }
 
 template <typename OpType, typename... Matchers>
 auto m_Op(Matchers... matchers) {
-  return detail::RecursivePatternMatcher<OpType, Matchers...>(matchers...);
+  return detail::RecursivePatternMatcher<Matchers...>(
+      OpType::getOperationName(), matchers...);
 }
 
 namespace matchers {
 inline auto m_Any() { return detail::AnyValueMatcher(); }
 inline auto m_Val(Value v) { return detail::PatternMatcherValue(v); }
 } // namespace matchers
 
 } // end namespace mlir
 
 #endif // MLIR_MATCHERS_H

mlir/lib/Dialect/StandardOps/Ops.cpp

 //===- Ops.cpp - Standard MLIR Operations ---------------------------------===//
 //
 // Part of the MLIR 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 "mlir/Dialect/StandardOps/Ops.h"
 
 #include "mlir/Dialect/CommonFolders.h"
 #include "mlir/IR/AffineExpr.h"
 #include "mlir/IR/AffineMap.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/Function.h"
 #include "mlir/IR/Matchers.h"
 #include "mlir/IR/Module.h"

 static void printStandardCastOp(Operation *op, OpAsmPrinter &p) {
   int stdDotLen = StandardOpsDialect::getDialectNamespace().size() + 1;
   p << op->getName().getStringRef().drop_front(stdDotLen) << ' '
     << *op->getOperand(0) << " : " << op->getOperand(0)->getType() << " to "
     << op->getResult(0)->getType();
 }
 
 /// A custom cast operation verifier.
-template <typename T> static LogicalResult verifyCastOp(T op) {
+template <typename T>
+static LogicalResult verifyCastOp(T op) {
   auto opType = op.getOperand()->getType();
   auto resType = op.getType();
   if (!T::areCastCompatible(opType, resType))
     return op.emitError("operand type ") << opType << " and result type "
                                          << resType << " are cast incompatible";
 
   return success();
 }
if (parser.parseOperandList(opInfos,
       parser.resolveOperands(opInfos, indexTy, operands))
     return failure();
   return success();
 }
 
 /// Matches a ConstantIndexOp.
 /// TODO: This should probably just be a general matcher that uses m_Constant
 /// and checks the operation for an index type.
-static detail::op_matcher<ConstantIndexOp> m_ConstantIndex() {
-  return detail::op_matcher<ConstantIndexOp>();
+static detail::op_matcher m_ConstantIndex() {
+  return detail::op_matcher(ConstantIndexOp::getOperationName());
 }
 
 //===----------------------------------------------------------------------===//
 // Common canonicalization pattern support logic
 //===----------------------------------------------------------------------===//
 
 /// This is a common class used for patterns of the form
 /// "someop(memrefcast) -> someop".  It folds the source of any memref_cast