This is an archive of the discontinued LLVM Phabricator instance.

[mlir-tblgen] Add DagNode StaticMatcher.
ClosedPublic

Authored by Chia-hungDuan on Jul 12 2021, 12:55 AM.

Details

Summary

Some patterns may share the common DAG structures. Generate a static
function to do the match logic to reduce the binary size.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Jul 12 2021, 12:55 AM
Chia-hungDuan requested review of this revision.Jul 12 2021, 12:55 AM

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?

jpienaar added inline comments.Jul 16 2021, 10:02 AM
mlir/tools/mlir-tblgen/RewriterGen.cpp
70

Comment?

299

isSrcPattern= ? (for consistency & to enable warnings on mismatch)

362

You can avoid indent(), <<, unindent, by doing

os.scope() << "return ::mlir::failure();\n";

1500

Could you document this one too?

Chia-hungDuan edited the summary of this revision. (Show Details)Aug 11 2021, 4:15 PM
Chia-hungDuan marked 4 inline comments as done.
Chia-hungDuan edited the summary of this revision. (Show Details)

Address Review Comment

Chia-hungDuan marked an inline comment as done.

Add comments

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?

Chia-hungDuan marked 9 inline comments as done.

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,

  1. Emit the static function for a DagNode
  2. Emit function call to a static function of 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.

jpienaar accepted this revision.Sep 9 2021, 2:59 PM

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

This revision is now accepted and ready to land.Sep 9 2021, 2:59 PM
Chia-hungDuan marked 8 inline comments as done.

Address review comments and refine test case(only check invariant)

Chia-hungDuan added inline comments.Sep 20 2021, 3:52 PM
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

This revision was automatically updated to reflect the committed changes.