This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mehdi_amini on Mar 26 2021, 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.Mar 26 2021, 2:12 PM
mehdi_amini requested review of this revision.Mar 26 2021, 2:12 PM
jpienaar accepted this revision.Mar 26 2021, 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
500

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.Mar 26 2021, 2:35 PM
mehdi_amini added inline comments.Mar 26 2021, 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
500

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.Mar 26 2021, 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.Mar 30 2021, 1:44 PM
mlir/lib/IR/Operation.cpp
186

!use_empty()?

mehdi_amini marked an inline comment as done.Mar 30 2021, 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