[Mlir-commits] [mlir] [MLIR] Fix ErasedOpsListener false positives for newly created ops/blocks (PR #192291)

[Amit Tiwari]

amitamd7 wrote:

> > Isn't this patch treating both "newly" created and "moved" ops as part of newlyCreatedOps? An already existing op that has moved is still something the walk should care for.
> 
> I'm not sure I get what you're getting at? Can you expand?
> 
> To be clear this whole patch is just changing an assertion, it not affecting any behavior. The check we're targeting is:
> 
> ```
>       llvm::report_fatal_error(
>           "unsupported erasure in WalkPatternRewriter; "
>           "erasure is only supported for matched ops and their descendants");
> ```
> 
> Basically the "erasure is only supported for matched ops and their descendants" is modified to not trigger on erasure in cases mentioned here.
> 
> The assertion is meant for protecting against these cases:
> 
> ```
> // Because we use walk-based pattern application, erasing the
> // op/block from the *next* iteration (e.g., a user of the visited op) is not
> // valid
> ```

I was trying to nitpick a scenario where the assertion might be weak i.e., If a pattern moves an existing op (triggering `notifyOperationInserted`) and then erases it within the same pattern, that's actually fine. My concerns should be taken careof as the walk driver won't successfully visit a moved-then-erased op anyway--the pattern has already restructured the IR. This is functionally the same situation as creating-then-erasing. 

https://github.com/llvm/llvm-project/pull/192291