[Mlir-commits] [mlir] [flang] [mlir][IR] Add rewriter API for moving operations (PR #78988)

[Matthias Springer]

================
@@ -366,3 +366,31 @@ void RewriterBase::cloneRegionBefore(Region &region, Region &parent,
 void RewriterBase::cloneRegionBefore(Region &region, Block *before) {
   cloneRegionBefore(region, *before->getParent(), before->getIterator());
 }
+
+void RewriterBase::moveOpBefore(Operation *op, Operation *existingOp) {
+  moveOpBefore(op, existingOp->getBlock(), existingOp->getIterator());
+}
+
+void RewriterBase::moveOpBefore(Operation *op, Block *block,
+                                Block::iterator iterator) {
+  Block *currentBlock = op->getBlock();
+  Block::iterator currentIterator = op->getIterator();
+  op->moveBefore(block, iterator);
+  if (listener)
+    listener->notifyOperationInserted(
+        op, /*previous=*/InsertPoint(currentBlock, currentIterator));
----------------
matthias-springer wrote:

We actually have the same issue with other kinds of IR changes:
* When erasing an operation, should we fire `notifyOperationModified` for the parent op? And for the parent's parent op? Etc.
* When inserting a newly created operation, should we fire `notifyOperationModified` for the parent op into which we are inserting? And for the parent's parent op? Etc.
* Same for erasing a block.

My conclusion was that we should not trigger a `notifyOperationModified` for things that have a separate notification. Otherwise, to be consistent, we would have to call `notifyOperationModified` for pretty much every IR change. That would potentially make it more difficult for listeners, because they would get duplicate notifications about the same IR change. (E.g., when inserting an op there would be two notifications.)

I think we should trigger `notifyOperationModified` only for "attribute changed", "property changed", "result type changed", "operand changed". Maybe also for "region entry block argument changed". And have separate callbacks for everything else.

Another thing that we could consider is giving `notifyOperationInsertion`, `notificationOperationRemoved`, `notifyBlockCreated`, etc. a default implementation that calls `notifyOperationModified`. Listeners could then decide what kind of granularity of notifications they would like to receive. (We already do something similar for `notifyOperationReplaced(Operation *, Operation *)`.)

> Is `notifyOperationInserted` really meant to handle arbitrary moves and the handler must handle all this?

I'd say that `notifyOperationInserted` should be called for all op insertions. Whether the inserted op is an already existing op or a newly created op is irrelevant. (Note the function is called `notifyOperationInserted` not `notifyOperationCreated`.)

But it raises the question whether `notifyOperationRemoved` should also be triggered. In my current implementation it is not, because the documentation of the callback says that it is triggered for "op erasure", not "op removal from a block". (I think the callback should be renamed to `notifyOperationErased`.)
```c++
/// Notify the listener that the specified operation is about to be erased.
/// At this point, the operation has zero uses.
virtual void notifyOperationRemoved(Operation *op) {}
```


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