This is an archive of the discontinued LLVM Phabricator instance.

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

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.Nov 18 2020, 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?

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!

mlir/include/mlir/IR/PatternMatch.h
13

Out of date now ;-)

mlir/lib/Rewrite/ByteCode.cpp
1051

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

1118

Add comment describing fusedLoc behavior with get below

1120

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

mlir/lib/Rewrite/ByteCode.h
103

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

mlir/test/Rewrite/pdl-bytecode.mlir
29

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). http://lab.llvm.org:8011/#/builders/13/builds/2128

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