Page MenuHomePhabricator

[mlir][PDL] Add support for PDL bytecode and expose PDL support to OwningRewritePatternList
Needs ReviewPublic

Authored by rriddle on Oct 9 2020, 2:15 AM.

Details

Summary

PDL patterns are now supported via a new PDLPatternModule class. This class contains a ModuleOp with the pdl::PatternOp operations representing the patterns, as well as a collection of registered C++ functions for native constraints/creations/rewrites/etc. that may be invoked via the pdl patterns. Instances of this class are added to an OwningRewritePatternList in the same fashion as C++ RewritePatterns, i.e. via the insert method.

The PDL bytecode is an in-memory representation of the PDL interpreter dialect that can be efficiently interpreted/executed. The representation of the bytecode boils down to a code array(for opcodes/memory locations/etc) and a memory buffer(for storing attributes/operations/values/any other data necessary). The bytecode operations are effectively a 1-1 mapping to the PDLInterp dialect operations, with a few exceptions in cases where the in-memory representation of the bytecode can be more efficient than the MLIR representation. For example, a generic AreEqual bytecode op can be used to represent AreEqualOp, CheckAttributeOp, and CheckTypeOp.

The execution of the bytecode is split into two phases: matching and rewriting. When matching, all of the matched patterns are collected to avoid the overhead of re-running parts of the matcher. These matched patterns are then considered alongside the native C++ patterns, which rewrite immediately in-place via RewritePattern::matchAndRewrite, for the given root operation. When a PDL pattern is matched and has the highest benefit, it is passed back to the bytecode to execute its rewriter.

Depends On D89104

Diff Detail

Event Timeline

rriddle created this revision.Oct 9 2020, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2020, 2:15 AM
rriddle requested review of this revision.Oct 9 2020, 2:15 AM

Still got some things to tidy up, but good for initial review.

First scan, mostly looks good thanks!

mlir/include/mlir/IR/PatternMatch.h
253

And we need these enable_if_t's due to the union used here?

308

And this array matches the first array? In using an ArrayAttr, it means these are uniqued, is there a reason to have them uniqued vs an array of them?

312

This has the same comment start as above, does this need to be expanded to differentiate it?

321

Could we have these in alphabetical order?

406

PDL?

mlir/lib/Rewrite/ByteCode.cpp
2

Not needed in cpp file.

103

GetOperandN ? So the above are "inlined" cases for common get operand queries? Does this actually pay off executionally/size wise?

mlir/lib/Rewrite/ByteCode.h
80

This is interesting. So this is just a raw memory block?

103

Nit: it need not be fused does it?

rriddle updated this revision to Diff 306278.Wed, Nov 18, 6:21 PM
rriddle marked 6 inline comments as done.

Resolve comments

mlir/include/mlir/IR/PatternMatch.h
253

Yes, given that we have to treat Value differently because of the nested unions.

308

It's more of what is a better interface to the user. They are always going to be in ArrayAttr form within PDL, so it is merely a question of is it better to pass in an ArrayRef(that is unwrapped from an ArrayAttr) or an ArrayAttr. I don't have a huge preference, but ArrayAttr has some nicer utilities for unwrapping the attributes.

mlir/lib/Rewrite/ByteCode.cpp
103

It helps a little bit in execution time, and more in the encoding given that it removes the need for two extra fields.

mlir/lib/Rewrite/ByteCode.h
80

Yeah. Given that everything is pointer based, we can just have one large block of opaque memory.

103

I used fused because it is an established thing that if you create a Fused loc with one location it just returns the location unfused. Would you prefer that I dropped "fused" here?