Differential D155127
[mlir][mlir-query] Introduce mlir-query tool with autocomplete support Authored by devajithvs on Jul 12 2023, 3:06 PM.
Details This commit adds the initial version of the mlir-query tool, which leverages the pre-existing matchers defined in mlir/include/mlir/IR/Matchers.h The tool provides the following set of basic queries: hasOpAttrName(string)
Diff Detail
Event TimelineComment Actions Thanks for the contribution! As a high-level remark, I would note that this is a bit monolithic where everything is in the tools, whereas in general we approach it from the point of view that the tool should be a minimal driver around a library implementation. Comment Actions Thanks Mehdi, I'll move all the implementation from the tools directory to keep it minimal, but where should it go? Comment Actions
A new library maybe? Also we discussed the possibility to reuse PDLL: what's missing from PDLL to use it instead of a custom language here? Comment Actions I see that orthogonal: that's a different input language. The initial usage was similar to clang-query where one constructs a matcher incrementally for use in C++. So you'd built and could then pretty much copy and paste reuse. Useful to do with PDLL too but not replacement IMHO. Comment Actions I wouldn't consider this orthogonal: this is a positioning of the tool, it's raison d'être here. I've never heard that the use case for this was "help the development of pattern matcher" for example. Is this really the main motivation? I'm questioning the need for this really. Comment Actions This is what I was doing when writing clang matchers quite a lot :) And a few blogs also point at it. While writing LLVM backends though it was more dumpr and equivalent to see trees, where I was simulating a workflow like this. I can't claim to have done a survey though. I don't think writing PDL by hand is yet as concise or familiar as this. So even if PDL was used to implement some of these, I see plain C++ matchers as very easy way to add these and very useful. I think PDL matching for regions or variadics is not as good yet. With PDL as input one can reuse a parser though.
Comment Actions Are you referring to PDL or PDLL? Can we get examples and comparisons here?
Comment Actions As for PDLL, let's take the query m hasOpName("arith.addf"). I'm not sure how the matcher implemented in pdll would look like, even though we can have something like the following, it's not a dynamic matcher (op-name need to be inferred at runtime). /// match is not supported, only rewriting/erasing supported Pattern HasOpNamePattern => match op<arith.addf>; As for PDL, let's say we have the following PDL matcher (created at runtime) in MLIR: module {
pdl.pattern @HasOpNamePattern : benefit(0) {
%0 = operands loc(#loc1)
%1 = types loc(#loc1)
%2 = operation "arith.addf"(%0 : !pdl.range<value>) -> (%1 : !pdl.range<type>) loc(#loc1)
match %2 /// match doesn't exist yet.
} loc(#loc)
}PDL has a rewrite/erase within the pattern and can simply load these patterns into a FrozenRewritePatternSet and then use applyPatternsAndFoldGreedily() However, for the mlir-query case, we only need to return matches. So, a separate implementation like applyPatternsAndReturnMatches() will be required, as I couldn't find an equivalent function that simply returns matches. This might not be the correct/best way to do this. Please let me know if there's a better way to do this. I'll try to find ways to decouple the parser from the matchers. Comment Actions
Thats a good idea, I'd be pro getting the basic structure correct and then generalizing (e.g., a bit of land and iterate). One easy way I could see here is to make the parsing as much an implementation detail as possible (not exposed in public headers etc). We could start with having a "parse to matcher" call that takes a string and returns a matcher representation that could then be fed to a match call which returns array of operation*. I think that is roughly then that the mlir-query driver registers matchers (well we could have default set and modify inside lib/ as the "language" is relatively fixed, but it feels like something outside lib for me), autocompletes/error checks in tool, calls the parse to match method (which will do redundant checks here probably as tool isn't only input and autocomplete + user experience is better with previous checking), calls the match and then walks the results printing them out. This would result in moving more parts out of lib/ and into tools/ I believe. Is that what you were thiking?
Comment Actions
Comment Actions During the last week, I've been looking at PDLL and trying to incorporate it with mlir-query. One issue I faced is that the Ahead-of-Time (AOT) compiled PDLL is not as powerful or flexible as the existing matchers we have. For example, it cannot match operations based on their names. As an example of what we can do with PDLL, let's consider an isConstantOp matcher, which can be defined as follows: Constraint IsConstantConstraint(op: Op) [{
return mlir::success(op->hasTrait<mlir::OpTrait::ConstantLike>());
}];
Pattern isConstantOp {
let op = op<>;
IsConstantConstraint(op);
erase op;
}This constraint is similar to what we have in regular MLIR matchers. I've separated the MatchFinder into a separate file to make it easier to add matchFinder support for PDLL Matchers. To add PDLL matcher support, we could update the MatchFinder with a function that accepts PDLByteCode and returns matches: static std::vector<Operation *> getMatches(Operation *root, const mlir::detail::PDLByteCode *bytecode) {
std::vector<Operation *> matches;
SmallVector<mlir::detail::PDLByteCode::MatchResult, 4> pdlMatches;
// Simple match finding with walk.
root->walk([&](Operation *op) {
bytecode->match(op, rewriter, pdlMatches, *mutableByteCodeState);
// ... Some logic to check if the size of pdlMatches has changed and append
// the corresponding match to matches appropriately ...
});
return matches;
}The parser will remain the same. As for the registry, it has a lookupMatcherCtor(StringRef MatcherName) function, which returns a matcherCtor (constructor) that helps infer the types and number of arguments and checks them against the parsed arguments (inside marshaller). Now constructMatcher takes this matcherCtor and the parsed argument values to return a VariantMatcher instance. The VariantMatcher here is created with the create function inside the implementation of the abstract class MatcherDescriptor (inside marshaller). The create method here takes the MLIR matcher function pointers, creates a DynMatcher with that, and then creates a VariantMatcher. If we are adding support for PDLL Matchers, we can add a new implementation of abstract MatcherDescriptor class for PDLByteCode. The idea for VariantMatcher class was borrowed from ASTMatchers and provides a nice abstraction for simple, variadic, and polymorphic matchers (since they have different types of nodes). In our case, we could utilize this abstraction for wrapped MLIR Matchers (called DynMatcher), variadic matchers (e.g., AllOf, AnyOf), and PDLMatchers (as PDLByteCode). Comment Actions We have something similar to a "parse to matcher" function, parseMatcherExpression returns the wrapped matcher (after going through all the mentioned steps). Inside the function, it calculates the VariantValue by parsing. The VariantValue has a method getMatcher() which returns the corresponding VariantMatcher. Once we have that (if we were to also do PDLL), we can check if it's a DynMatcher or a PDLMatcher and handle it appropriately. EDIT: I'm working on the review comments, I'll make an update again with that and tests, which I missed. Comment Actions Move all internal details to internal namespace
Comment Actions Thanks for all the changes!
Comment Actions I'm working on the changes now.
Comment Actions
Comment Actions
Comment Actions Thanks for reviewing :), I've made the suggested changes.
Comment Actions I've rebased it and ran the tests against the latest llvm version. Everything works without needing any change. It would be great to have this merged if everything is ok (before phabricator is made a read only HTML copy) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
You'll need file type specifier here to differentiate between C & C++