This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Introduce applyOpPatternsAndFold for op local rewrites
ClosedPublic

Authored by bondhugula on Apr 4 2020, 10:12 PM.

Details

Summary

Introduce mlir::applyOpPatternsAndFold which applies patterns as well as
any folding only on a specified op (in contrast to
applyPatternsAndFoldGreedily which applies patterns only on the regions
of an op isolated from above). The caller is made aware of the op being
folded away or erased.

Depends on D77485.

Diff Detail

Event Timeline

bondhugula created this revision.Apr 4 2020, 10:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
rriddle requested changes to this revision.Apr 4 2020, 10:53 PM
rriddle added inline comments.
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
251

This is wrong, you don't handle when the op is erased by a pattern. This really feels like should be its own driver, it doesn't really share anything.

This revision now requires changes to proceed.Apr 4 2020, 10:53 PM
bondhugula added inline comments.Apr 5 2020, 12:30 AM
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
251

Hmm... if the op was erased in a pattern by just erase(), there is no way to know if it was erased? If the op is erased via eraseOp or replaceOp, checking for null at the op's position via worklistMap will address that (we'll have to just keep the op in the worklist before matchAndRewrite). The other issue is that the erased op's operands will also get into the worklist and we don't want them to, but this can be addressed by clearing out the worklist. This could use a separate driver as you say, but it doesn't immediately address the first issue - how do we detect erasure if done via op.erase()?

rriddle added inline comments.Apr 5 2020, 12:37 AM
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
251

how do we detect erasure if done via op.erase()?

We don't have to. It isn't valid to do IR mutations outside of the PatternRewriter.

I thought this was documented somewhere, but https://mlir.llvm.org/docs/QuickstartRewrites/ seems to be extremely out of date. I have an in-progress update to https://mlir.llvm.org/docs/Canonicalization/ I'll add that in as part of it.

Implementing this functionality has hit a dead-end; there's no easy way out.

bondhugula retitled this revision from [MLIR] Introduce applyOpPatternsAndFold for op local rewrites to [MLIR] [WIP] Introduce applyOpPatternsAndFold for op local rewrites.Apr 5 2020, 12:39 AM
bondhugula updated this revision to Diff 255135.Apr 5 2020, 5:00 AM

Update to use worklist free driver. This isn't still well tested - so could
wait till it could be used at a few places.

bondhugula updated this revision to Diff 255137.Apr 5 2020, 5:05 AM

Fix some gratuitous changes. This isn't still well-tested and could wait for
some of the pending revisions to get in.

Harbormaster completed remote builds in B51828: Diff 255135.
bondhugula updated this revision to Diff 255154.Apr 5 2020, 6:53 AM

rebase and permute commit stack

bondhugula marked an inline comment as done.Apr 6 2020, 9:44 AM
bondhugula added inline comments.
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
251

I don't recall that requirement ever being documented, although at one point some instances of op.erase() were replaced by rewriter.eraseOp(op). The document itself hasn't changed core content wise since Mar 2019. I couldn't find it in your recent update to the canonicalization doc. Is another one pending?

If that's the constraint on how one can erase, then I think I have it implemented correctly now. It needs better testing though.

bondhugula retitled this revision from [MLIR] [WIP] Introduce applyOpPatternsAndFold for op local rewrites to [MLIR] Introduce applyOpPatternsAndFold for op local rewrites.Apr 8 2020, 2:55 AM
bondhugula edited the summary of this revision. (Show Details)

Can you add test coverage?

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
251

Yes, there is another one pending.

254

Can you use a comment block to split these?

257

"bottom up" doesn't really make sense here.

266

Please use /// for the top-level comments.

330
while (!erased && changed && ...)

?

rriddle requested changes to this revision.Apr 9 2020, 11:15 AM
This revision now requires changes to proceed.Apr 9 2020, 11:15 AM
bondhugula marked 10 inline comments as done.

Address review comments.

Thanks for the review.

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
254

Not sure I understood it right - but PTAL at what I did.

330

That won't be accurate since although you'll correctly exit the iteration and return, you will be returning !changed, which will be false instead of being true (i.e., the driver has converged).

rriddle requested changes to this revision.Apr 10 2020, 11:53 PM

Can you add test coverage?

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
330

Ah yeah, duh. Thanks.

This revision now requires changes to proceed.Apr 10 2020, 11:53 PM

Can you add test coverage?

Sure; I'm trying to find a good set of places to call this from. Thanks for the review.

Add testing.

bondhugula marked an inline comment as done.Apr 13 2020, 12:31 PM

Drop unrelated changes.

Harbormaster failed remote builds in B52979: Diff 257073!
bondhugula added a comment.EditedApr 14 2020, 8:06 PM

Ping @rriddle - do you have more comments here? I added test cases and the output parameter on the method has been made optional. (A test case that exercises the op being erased is in the child revision. )

Nice!

I expect this to be very useful for more easily expressing fused passes.

Comment updates - reset erased to false in driver's simplifyLocally.

rriddle accepted this revision.Apr 14 2020, 11:34 PM

I have a growing concern that rebuilding the pattern list is going to become extremely costly for compile time. I don't have a concrete suggestion at the moment, but we should avoid rebuilding if possible.

That being said, I think this is a great start to doing local canonicalization. Thank you for pushing this!

mlir/include/mlir/IR/PatternMatch.h
464

I'm of the opinion that we should remove the return value from these methods. It is not used anywhere, and it isn't clear what the user would use it for in any of the existing usages.

mlir/lib/Dialect/Affine/Transforms/AffineDataCopyGeneration.cpp
233

This seems extremely costly. Can we create these once and just apply them if necessary?

mlir/lib/Dialect/Affine/Transforms/SimplifyAffineStructures.cpp
98

Same here. Can we avoid rebuilding a pattern set for each operation?

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
275

Please use /// here an below.

mlir/lib/Transforms/Utils/LoopUtils.cpp
318

nit: Use // here.

mlir/test/lib/Dialect/Affine/TestAffineDataCopy.cpp
100

nit: isa<>() || isa<>()

This revision is now accepted and ready to land.Apr 14 2020, 11:34 PM

NFC - clean up erasure logic

clean up and fix erasure further

bondhugula marked 8 inline comments as done.

Address review comments

mlir/include/mlir/IR/PatternMatch.h
464

The child revision has the use case where it's essential - let's discuss there if there are alternatives.

mlir/lib/Dialect/Affine/Transforms/SimplifyAffineStructures.cpp
98

Sure - done.

This revision was automatically updated to reflect the committed changes.