[PATCH] D72249: Detemplaize m_Op and RecursivePatternMatcher.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:200
+static detail::op_matcher m_ConstantIndex() {
+  return detail::op_matcher(ConstantIndexOp::getOperationName());
 }
----------------
rriddle wrote:
> 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
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 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72249/new/

https://reviews.llvm.org/D72249