Page MenuHomePhabricator

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

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



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.

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)


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

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.


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

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


mehdi_amini marked an inline comment as done.Tue, Mar 30, 2:10 PM
mehdi_amini added inline comments.

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


Here @ftynse FYI

Address river's comment