Page MenuHomePhabricator

[mlir:PDL] Add support for DialectConversion with pattern configurations
ClosedPublic

Authored by rriddle on Sep 1 2022, 12:37 PM.

Details

Summary

Up until now PDL(L) has not supported dialect conversion because we had no
way of remapping values or integrating with type conversions. This commit
rectifies that by adding a new "pattern configuration" concept to PDL. This
essentially allows for attaching external configurations to patterns, which
can hook into pattern events (for now just the scope of a rewrite, but we
could also pass configs to native rewrites as well). This allows for injecting
the type converter into the conversion pattern rewriter.

Diff Detail

Event Timeline

rriddle created this revision.Sep 1 2022, 12:37 PM
rriddle requested review of this revision.Sep 1 2022, 12:37 PM

The type converter scopes is a bit new, is this solely for this or where else would you see this be used?

mlir/lib/IR/PatternMatch.cpp
194

This is creative use of bookkeeping :-) Is this due to invalidation of Value hash maps ? (E.g.,. Why couldn't this just be an Analysis that gets populated)

mlir/lib/Transforms/Utils/DialectConversion.cpp
3300

This is a bit undesirable as this is at runtime. And this could be for a dynamically compiled pattern which gets invoked from a pass that can actually flag a failure. I'd rather have an error propagated.

rriddle added inline comments.Sep 7 2022, 5:08 PM
mlir/lib/IR/PatternMatch.cpp
194

That gives me an idea, let me rework this.

mlir/lib/Transforms/Utils/DialectConversion.cpp
3300

Let me rework this then. We'll have to build failable rewrites into PDL, which will require some more substantial changes. I'll start building that under this.

rriddle updated this revision to Diff 458913.Sep 8 2022, 4:43 PM
rriddle edited the summary of this revision. (Show Details)
rriddle updated this revision to Diff 458918.Sep 8 2022, 4:59 PM
rriddle marked 2 inline comments as done.

The type converter scopes is a bit new, is this solely for this or where else would you see this be used?

Removed it, thanks. We used to need that, but I hadn't checked in a while.

rriddle updated this revision to Diff 460540.Sep 15 2022, 4:27 PM
jpienaar accepted this revision.Oct 6 2022, 6:53 PM

Nice, thanks! Haven't tried it out yet though :)

mlir/include/mlir/IR/PatternMatch.h
881

s/and/or/ ?

1250

So these could both fail but still be run and success returned? What should be expectation ? (fail if one fails, fails if both fails, shortcircuit return or evaluate both)

mlir/include/mlir/Transforms/DialectConversion.pdll
2

pdll ?

mlir/lib/Transforms/Utils/DialectConversion.cpp
3279

This pulls PDL into DialectConversion proper, any cost if PDL is not used? Could these be in a helper PDL side?

mlir/test/lib/Transforms/TestDialectConversion.pdll
16

OOC when would one not want the result types converted if there is a type converter registered? (I believe dialect conversion side that's implicit too). Is this to avoid making dialect conversion/type converter too ingrained in PDL? (although as you've done it above it is a config only).

I could see this maybe differ for intermediate results. But can't think of good example. (not a blocker, explicit "default" is good)

This revision is now accepted and ready to land.Oct 6 2022, 6:53 PM
rriddle marked 5 inline comments as done.Mon, Nov 7, 4:23 PM
rriddle added inline comments.
mlir/include/mlir/IR/PatternMatch.h
1250

Originally I wasn't expecting nested logicalresult/failable things within pair/tuple. Just switched this to properly check for failure though (just in case).

mlir/lib/Transforms/Utils/DialectConversion.cpp
3279

No cost if it's unused. The class is just defined in DialectConversion, it's attached on the PDL patterns directly so you only pay a cost if you're using a PDL pattern that wants dialect conversion stuff.

mlir/test/lib/Transforms/TestDialectConversion.pdll
16

Most of the cases you wouldn't want result type conversion is for intermediate results. I'm a bit hesitant on implicit behavior changes here because then the underlying behavior would likely become harder to understand. If we could come up with a good model for it I'd be okay, but right now IMO being explicit is a better starting point (and matches the C++ side).

rriddle updated this revision to Diff 473847.Mon, Nov 7, 6:25 PM
rriddle marked 3 inline comments as done.