In the light of this patch [mlir][IR] Rename "update root" to "modify op" in rewriter API by matthias-springer · Pull Request #78260 · llvm/llvm-project · GitHub about the rewriter root update. I am still wondering if I use it properly.
Based on Pattern Rewriting : Generic DAG-to-DAG Rewriting - MLIR
All IR mutations, including creation, must be performed by the given PatternRewriter
That much is pretty clear. What I am less sure about is the extent of the changes that can be done, in a PatternRewriter, to operations that are not the “root” (the matched one based on the new definition)
In the case of replaceOp(root, newOp), it is not just the rootOp that is modified but all its users so it seems that it is ok to, at least indirectly, modify other ops.
Maybe the new Op, takes new arguments, so an additional op has to be created, maybe in the same block or maybe in a parent region. Is it ok, so long as it is done by the rewriter?
A more extreme case would be if replaceOp were to be called on an op that is not the root. Would it be safe, to directly modify a sibling or child or a parent region? That’s not super clear.
I don’t think I ever tried this case, I am just mentioning it because if we can do that then I think we can pretty much do anything at this point.
The case our team is unsure about, is if we want to move a sibling of the root into the root itself, which has regions. mlir::Operation has moveBefore/moveAfter but we know not to use them directly because it is not using the Rewriter’s API. We are also not just modifying the root but also removing a sibling op.
Is it just a matter of having mlir::Operation::moveBefore inserted between startOpModification/finalizeOpModification ?
And if it is, on which op should it be called ? Both maybe?
Thank you
There shouldn’t be any restriction inherent to the PatternRewrite infrastructure, as long as an API is exposed by the Rewriter. I don’t see anything in your description that shouldn’t be allowed, but for moving an operation (whether it is the root or not).
Now we got there incrementally, hence a lot of the current fuzziness. In particular we have the concept of “patterns” and the associated driver. We also had to differentiate the “dialect conversion” (and thus special ConversionPattern patterns) from the “greedy” pattern application used in canonicalization.
One of the key aspect of the DialectConversion framework is the ability to roll-back a pattern application to a previous state, which limits the kind of transformations that can be done in a pattern.
See OperationTransactionState for the transactional aspect of in-place op modification for the dialect conversion, and note how it can’t handle rolling back an op that would be moved (actually I just noticed how it isn’t handling properties very well either, we should fix this).
In the greedy rewriter on the other hand, the use of the rewriter for performing IR change grew over time in a best-effort fashion in order to help with the convergence of the fix-point algorithm: tracking modification allows re-enqueuing operations as we go. For this, not using the rewriter for modification wasn’t critical, it would just lead to spurious iterations.
However some pattern application developed the need to track modifications a bit more closely, for example the Transform dialect is injecting a “listener” to track changed in the IR: TrackingListener .
Because of that the contract evolved to be more strict and requiring that all modifications in a pattern are performed through the rewriter.
Coming back to the problem of moving an operation: I think this shouldn’t be a problem in most cases (rolling back in dialect conversion wouldn’t work), however you need to think about what is modified in-place here: the op that is moved is obvious but what about its parent? What about the parent of the region where you move it? And all the enclosing op in between?
There may be listeners that would be sensitive to these modifications unfortunately, and we would be better off adding a specific API for moving it instead of considering it a valid “in-place modification”.
And really: how can it be “in-place” when the whole point is to change the “place” it is at 
Thank you. I have seen places where loops are rewritten by calling startOpModification on both the original and the new one. I think, I will just do the same for the move then.