Page MenuHomePhabricator

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

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



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!


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


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?


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


Could we have these in alphabetical order?




Not needed in cpp file.


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


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


Nit: it need not be fused does it?

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

Resolve comments


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


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.


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


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


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?

jpienaar accepted this revision.Nov 30 2020, 5:15 PM

Nice, a bit big change, but not easy to break up. I think it looks good in general, thanks!


Out of date now ;-)


How about adding the enum value into the debug message or just doing GetOperandX ?


Add comment describing fusedLoc behavior with get below


Mmm, this creates a sequence of uniqued fusedloc, even though you only care about the final one. Could this be avoided?


Well I was also thinking of location directives we have in DRR (so yes dropping fused)


Nit: could we have this be something easier to see? E.g., test.replaced_by_pattern? When I read it first I though success below just indicated something about the rewriter, not that it was the op created here

This revision is now accepted and ready to land.Nov 30 2020, 5:15 PM
rriddle updated this revision to Diff 308774.Dec 1 2020, 2:30 PM
rriddle marked 9 inline comments as done.

Resolve comments

Thanks for the review Jacques! Very appreciated. Should be much easier to iterate on PDL now.

This revision was landed with ongoing or failed builds.Dec 1 2020, 3:06 PM
This revision was automatically updated to reflect the committed changes.

This appears to still be failing on VS2017 after the latest GCC5 fix (although builder is having some trouble, so I'll confirm).

E:\build_slave\mlir-x64-windows-ninja\llvm-project\mlir\lib\Rewrite\ByteCode.cpp(532): error C2440: '<function-style-cast>': cannot convert from 'mlir::pdl_interp::BranchOp' to 'mlir::SuccessorRange'
E:\build_slave\mlir-x64-windows-ninja\llvm-project\mlir\lib\Rewrite\ByteCode.cpp(532): note: No constructor could take the source type, or constructor overload resolution was ambiguous
E:\build_slave\mlir-x64-windows-ninja\llvm-project\mlir\lib\Rewrite\ByteCode.cpp(640): error C2440: '<function-style-cast>': cannot convert from 'mlir::pdl_interp::RecordMatchOp' to 'mlir::SuccessorRange'
E:\build_slave\mlir-x64-windows-ninja\llvm-project\mlir\lib\Rewrite\ByteCode.cpp(640): note: No constructor could take the source type, or constructor overload resolution was ambiguous