Page MenuHomePhabricator

[mlir] Detemplaize m_Op and RecursivePatternMatcher.
Needs RevisionPublic

Authored by chelini on Jan 6 2020, 1:32 AM.

Details

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.

Diff Detail

Event Timeline

chelini created this revision.Jan 6 2020, 1:32 AM

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 requested changes to this revision.Jan 6 2020, 5:22 AM

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

This revision now requires changes to proceed.Jan 6 2020, 5:22 AM
chelini marked an inline comment as done.Jan 6 2020, 7:34 AM
chelini added inline comments.
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 requested changes to this revision.Jan 6 2020, 8:11 AM
rriddle added inline comments.
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 added inline comments.Jan 6 2020, 8:15 AM
mlir/include/mlir/IR/Matchers.h
54

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

ftynse added inline comments.Jan 8 2020, 6:25 AM
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 ?

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,

flaub added a subscriber: flaub.Mar 6 2020, 11:44 AM
lebedev.ri retitled this revision from Detemplaize m_Op and RecursivePatternMatcher. to [mlir] Detemplaize m_Op and RecursivePatternMatcher..Mar 6 2020, 12:11 PM
nicolasvasilache resigned from this revision.Oct 2 2020, 1:25 AM

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

Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 1:25 AM