Some patterns may share the common DAG structures. Generate a static
function to do the match logic to reduce the binary size.
Details
- Reviewers
jpienaar antiagainst - Commits
- rGbb2506061b06: [mlir-tblgen] Add DagNode StaticMatcher.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Could you add the impact of this change to the description too?
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
---|---|---|
229 | Could you add comments for these? |
Add a data as reference,
The pattern in tensorflow lit(tensorflow/compiler/mlir/lite/transforms/optimize_patterns.td), the generated file size is reduced from 1160004 byte to 932889 byte, about 19% reduction and there's no difference in compilation time. Will do more detailed profiling.
Nice, any way to test this locally?
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
---|---|---|
70 | param name? | |
227 | Why not pass the RecordOperatorMap by reference too? Is it allowed to null? | |
281 | If memory serves this form is problematic with some C++14 compilers (works in C++17) | |
285 | Not sure, I follow. This description makes it sound like a global topological ordering of DagNodes. But for different patterns the DagNodes are independent, so don't know what it means in that case. | |
293 | Could you add how this is used? | |
310 | /*depth=*/ ? | |
340 | Funnily I consider that we don't support specifying constraint such for op results too as a gap that folks need to work around, so I'd rather say that names are specified both to bind and to provide constraints. So collecting is required in general as names have 2 uses. | |
348 | Same | |
350 | This comment feels out of place, the above one says collecting all bound symbols and loop below does that, not sure how this relates to that. | |
360 | formatv doesn't seem to be doing anything here | |
373 | Is another way of saying this, that as we consider nodes in topological order it will only be empty when one needs to emit the static match call? ("theoretically" is throwing me off here, makes it feel like there is some corner case where this could fail but I don't think that is the intention) | |
482 | Which test exercised it? | |
483 | Is it guaranteed to be an op here? Also could we avoid this name being hardcoded here? (perhaps making line 485 a lambda to be called from here and there?) | |
1482 | certain Dag? | |
1505 | Ohhh so refStats is used for computing the order rather than ref as in reference counting/memory. I didn't think of that at definition site. | |
1528 | Would this also generate matchers for non-duplicate DagNode? |
Address review comments and add a test
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
---|---|---|
227 | This is just to align the usage in PatternEmitter PatternEmitter(Record *pat, RecordOperatorMap *mapper, raw_ostream &os StaticMatcherHelper &helper); And the use of mapper are expecting pointer parameter. | |
281 | Do you have more details? I see similar form in framework like, -- BuiltinTypes.h -- class ShapedType : class ShapedType : public Type { public: using Type::Type; // TODO: merge these two special values in a single one used everywhere. // Unfortunately, uses of `-1` have crept deep into the codebase now and are // hard to track. static constexpr int64_t kDynamicSize = -1; static constexpr int64_t kDynamicStrideOrOffset = std::numeric_limits<int64_t>::min(); ... } | |
285 | If two DagNode in two patterns are identical, they will share the same llvm::DagInit. The reason we need the topological sort is, suppose we have two patterns DAGRootA | DAGA | DAGC DAGRootB | DAGA | DAGC DAGA and DAGC are the common parts between them. When we emit matching logic for DAGA, if DAGC has a static matcher generated, then it'll be a function call in DAGA, otherwise it'll inline the matching logic of DAGC. In this case, inlining is not our expectation, to avoid that, we apply the topological order to ensure all the dependents are generated. We could have different approaches to do that, this way is the one I think it gives the least impact in the current structure, e.g., impact to PatternEmitter. I can give more details during our chat. | |
340 | Minor updated the description. | |
350 | Here are two cases we need to collect symbols in a DagNode,
In 1, we will do things like, pattern.collectBoundSymbols(tree, symbolInfoMap, /*isSrcPattern=*/true); symbolInfoMap.assignUniqueAlternativeNames(); In 2, the only overlap is pattern.collectBoundSymbols(tree, symbolInfoMap, /*isSrcPattern=*/true); The reason it doesn't need assignUniqueAlternativeNames is we get the alternative name from global SymbolInfoMap. The comment here is just to indicate that we are not missing calling assignUniqueAlternativeNames like other locations. Do you think we need to remove it? | |
373 | You're right. That makes people think there's a corner case for topological order. What I want to mention here is, if staticMatcherHelper.useStaticMatcher(node) returns true for a dependent DagNode, then we can just generate the function call to match the dependent DagNode. The exception is when we are going to generating the static function for a DagNode itself. Comment updated. Let me know if it's better | |
482 | https://github.com/llvm/llvm-project/blob/main/mlir/tools/mlir-tblgen/RewriterGen.cpp#L1133 if (numResults != 0) { for (int i = 0; i < numResults; ++i) os << formatv("for (auto v: castedOp0.getODSResults({0})) {{\n" " tblgen_types.push_back(v.getType());\n}\n", resultIndex + i); } Do you think we need to fix this hard code usage? | |
483 | Done. | |
1528 | No, only duplicate DagNodes do. |
Nice, thanks!
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
---|---|---|
222 | s/Exam/Examine/ , or (perhaps) Tracks DagNode's referenced multiple times across patterns. | |
223 | Perhaps: Enables generating static matcher functions for DagNode's referenced multiple times rather than inlining them. | |
227 | I think these may be leftover from when we followed a slightly different convention here. I'm pro making these references as we aren't checking for null anywhere | |
235 | the name of the static DAG matcher function corresponding to the node. ? | |
241 | The interface is for adding an individual Record. Record is the raw TableGen type, is it difficult to use something more specific here? These records represent patterns correct? Could we just say pattern | |
246 | Do we need ostream up until this point? E.g., why not have populateStaticMatchers(raw_ostream& os) and remove it from the (now explicit) class constructor? | |
266 | Could you expand comment to capture that this is a counter used/updated continuously to generate unique names? | |
281 | Sure, https://en.cppreference.com/w/cpp/language/static "a constexpr static data member (since C++11)(until C++17) is odr-used, a definition at namespace scope is still required", although I don't think it would hit here as inside the cpp one. | |
285 | I like this example. Could you add this to the function or class document? So basically, by handling the nodes in order of nesting we can avoid accidentally inlining. | |
350 | Yes I think better to remove, thanks | |
482 | Oh, I meant more: do we have a test that exercises this path? E.g., you have this comment describing a case to avoid, do we have a unit test for it |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
---|---|---|
241 | Changed to addPattern. Yes, it looks better to say it pattern. I think it may be a legacy naming. | |
246 | Done. it looks better now. | |
281 | Got it! Besides it's inside cpp, according to the definition of odr-used, https://en.cppreference.com/w/cpp/language/definition Informally, an object is odr-used if its value is read (unless it is a compile time constant) or written, its address is taken, or a reference is bound to it; ... I think it's safe here | |
482 | We have a test checking that, https://github.com/llvm/llvm-project/blob/main/mlir/test/mlir-tblgen/rewriter-indexing.td#L40 But before running the test, it'll fire the compilation error because the result pattern will use castedOp0 |
Comment?