Page MenuHomePhabricator

Fix deletion of operations through the rewriter in a pattern matching a consumer operation
ClosedPublic

Authored by mehdi_amini on Fri, Mar 26, 2:12 PM.

Details

Summary

This allows for the conversion to match A(B()) -> C() with a pattern matching
A and marking B for deletion.

Also add better assertions when an operation is erased while still having uses.

Diff Detail

Event Timeline

mehdi_amini created this revision.Fri, Mar 26, 2:12 PM
mehdi_amini requested review of this revision.Fri, Mar 26, 2:12 PM
jpienaar accepted this revision.Fri, Mar 26, 2:35 PM
jpienaar added a subscriber: jpienaar.
jpienaar added inline comments.
mlir/lib/Transforms/Utils/DialectConversion.cpp
919–923

So if we did a topological order before deletion this would not be needed? (Not saying we should just to double check I understood what would have avoided the need for drop all uses)

mlir/test/lib/Dialect/Test/TestPatterns.cpp
502

And the failure was specific to this order? Or either order if erase would have worked? (If the latter it would be good to document it here too even though it is an implementation detail, without the context of this change the test is difficult to interpret)

This revision is now accepted and ready to land.Fri, Mar 26, 2:35 PM
mehdi_amini added inline comments.Fri, Mar 26, 2:47 PM
mlir/lib/Transforms/Utils/DialectConversion.cpp
919–923

Yes this is correct. Right now the usual expectation is that the developer should delete all users before deleting the producer.
But because of this reverse here it kind of break the usual idiom...

Right now a user could use the rewriter to mark eraseOp on the producer before the users, but I don't think that's desirable.

mlir/test/lib/Dialect/Test/TestPatterns.cpp
502

Yes the failure is specific to this order.
As mentioned in the order comment thread, when you manipulate the IR directly you need to delete the consumers before the producer.
However because calling eraseOp is "scheduling for later", the rewrite is doing it in reverse order.

Writing the code with the eraseOp in the other direction would work without my change in the rewriter right now.

ftynse added inline comments.Fri, Mar 26, 3:18 PM
mlir/lib/Transforms/Utils/DialectConversion.cpp
923

Would it make sense to also have a check under NDEBUG that all users of this operation are also scheduled to be dropped? Otherwise be could be dropping operands of operations that remain live and won't catch it (previously, we would catch that in the destructor of operations because there were remaining uses).

Check if trackedOps is null before using it

rriddle added inline comments.Tue, Mar 30, 1:44 PM
mlir/lib/IR/Operation.cpp
186

!use_empty()?

mehdi_amini marked an inline comment as done.Tue, Mar 30, 2:10 PM
mehdi_amini added inline comments.
mlir/lib/Transforms/Utils/DialectConversion.cpp
923

Actually we already check this below and fails the legalization in this case.

2351

Here @ftynse FYI

Address river's comment