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.
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.
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)
This looks like unrelated change, and also wrong format.
We don't prefix StringRef with llvm, it's re-exported into the mlir namespace.
Please add explicit
return operationToMatch == op->getName().getStringRef()
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.
Can you just inline this function into the call place after removing the unneeded if?
Same as above
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?
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:
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 ?