Page MenuHomePhabricator

[mlir] Remove ConversionPatternRewriter recursive optimization.
AbandonedPublic

Authored by tpopp on Oct 21 2020, 6:12 AM.

Details

Reviewers
rriddle
Summary

Previously replaced ops would have all children with regions marked
as ignored also. This removes that change as it then erroneously ignored
regions that were later moved to other unignored/replaced regions.

This change might hurt performance of the ConversionPatternRewriter
by now checking ancestral ops to see if this op is "dangling" due to
any of those ops being removed, but should not be a driving performance
loss.

Note, this is not an NFC optimization removal because now some ops
that were being ignored due to a replace-and-move situation are not
ignored, and additionally, children of replaced ops were previously not
ignored in the optimization scheme but now will be if they are dangling.

Diff Detail

Event Timeline

tpopp created this revision.Oct 21 2020, 6:12 AM
tpopp requested review of this revision.Oct 21 2020, 6:12 AM
tpopp added a comment.Oct 21 2020, 6:25 AM

Let me know what you think of something like this River.

I was having issues where OpA containing OpB which contains OpC would not legalize OpC if OpA was changed and OpB was not.
Specifically, this code would result in inner ops being ignored because they were in the old op when Replace was called:

rewriter.replaceOp(assumingOp, newAssumingOp.getResults());
rewriter.inlineRegionBefore(assumingOp.doRegion(), newAssumingOp.doRegion(),
                            newAssumingOp.doRegion().end());

When looking at this more, I did not see any clean way to keep this optimization across different use cases of this method. It's hard to concisely explain. Let me know if you would like to discuss over a short video chat or similar.

rriddle added a comment.EditedOct 21 2020, 6:03 PM

Let me know what you think of something like this River.

I was having issues where OpA containing OpB which contains OpC would not legalize OpC if OpA was changed and OpB was not.
Specifically, this code would result in inner ops being ignored because they were in the old op when Replace was called:

rewriter.replaceOp(assumingOp, newAssumingOp.getResults());
rewriter.inlineRegionBefore(assumingOp.doRegion(), newAssumingOp.doRegion(),
                            newAssumingOp.doRegion().end());

This looks like an anti-pattern and only really works because of the internal implementation details of the conversion rewriter, i.e. if a pattern did this outside of the conversion infra it would segfault. replaceOp and eraseOp should be treated as the end of the lifetime for that operation and patterns shouldn't rely on the existence of the erased/replaced op(including its internals) after those calls. Given that, we don't intend to support these types of use cases and should ideally add some asserts to catch them early. Is this situation what you were running into or was it just similar?

When looking at this more, I did not see any clean way to keep this optimization across different use cases of this method. It's hard to concisely explain. Let me know if you would like to discuss over a short video chat or similar.

tpopp abandoned this revision.Wed, Oct 28, 10:25 AM

Thank you for the explanation. My first example was the example, so I've reworked the code to move data before replacing an op. I will follow up with adding an assert to help prevent other people making the same mistake as me.

Also, just context on how I thought of this working in case it helps with any other decisions. When I thought of rewriter.replaceOp(Old, New), my assumption was that this replaced the uses of Old with New, but left the the op around until it was cleaned up later. With this model, I thought it was safe to continue using the Old op after replacing it.

Thank you for the explanation. My first example was the example, so I've reworked the code to move data before replacing an op. I will follow up with adding an assert to help prevent other people making the same mistake as me.

Also, just context on how I thought of this working in case it helps with any other decisions. When I thought of rewriter.replaceOp(Old, New), my assumption was that this replaced the uses of Old with New, but left the the op around until it was cleaned up later. With this model, I thought it was safe to continue using the Old op after replacing it.

Ah, yeah that would explain it. Do you see any gaps in documentation that could make this more clear? Either on the PatternRewriter class definition or in https://mlir.llvm.org/docs/PatternRewriter/#pattern-rewriter?