Add Operation::moveAfter

[GMNGeoffrey]

Summary

This revision introduces an Operation::moveAfter mirroring
Operation::moveBefore to move an operation after another
existing operation or an iterator in a specified block.

Resolves https://bugs.llvm.org/show_bug.cgi?id=45799

[GMNGeoffrey]

mlir/lib/IR/Operation.cpp

495

So now is not the time to fix it, but the LLVM style guide specifically says not to duplicate all the comments in the implementation. I think MLIR does this a lot. Can we remove these?

Don’t duplicate the documentation comment in the header file and in the implementation file. Put the documentation comments for public APIs into the header file. Documentation comments for private APIs can go to the implementation file. In any case, implementation files can include additional comments (not necessarily in Doxygen markup) to explain implementation details as needed.

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

[GMNGeoffrey]

Mehdi, can you land this?

[GMNGeoffrey]

Pardon my phabricator ignorance, but why is the comment here "This revision was not accepted when it landed; it landed in state Needs Review" immediately below "mehdi_amini accepted this revision"? Is that because River is marked as a blocking reviewer? Seems like a fragile system to have one person be the only one who can approve some set of code. What if they go on vacation?

[mehdi_amini]

In D79640#2027782, @GMNGeoffrey wrote:

Pardon my phabricator ignorance, but why is the comment here "This revision was not accepted when it landed; it landed in state Needs Review" immediately below "mehdi_amini accepted this revision"? Is that because River is marked as a blocking reviewer? Seems like a fragile system to have one person be the only one who can approve some set of code. What if they go on vacation?

The email just indicates that River set himself as "blocking reviewer" on this revision and he didn't review it.

[bondhugula]

In D79640#2027782, @GMNGeoffrey wrote:

Pardon my phabricator ignorance, but why is the comment here "This revision was not accepted when it landed; it landed in state Needs Review" immediately below "mehdi_amini accepted this revision"? Is that because River is marked as a blocking reviewer? Seems like a fragile system to have one person be the only one who can approve some set of code. What if they go on vacation?

This is a good point. Would someone have access / permissions to substitute a blocking reviewer in such cases when the blocking reviewer may not be available?

[mehdi_amini]

Anyone can edit a revision I think? You should be able to do it, so is the revision author I think.

[mehdi_amini]

@GMNGeoffrey : this is morally equivalent to WANT_LGTM in Google's code review system I think.

[rriddle]

In D79640#2027782, @GMNGeoffrey wrote:

Pardon my phabricator ignorance, but why is the comment here "This revision was not accepted when it landed; it landed in state Needs Review" immediately below "mehdi_amini accepted this revision"? Is that because River is marked as a blocking reviewer? Seems like a fragile system to have one person be the only one who can approve some set of code. What if they go on vacation?

In general if you are touching something and the code owner is on vacation you wait for them to get back. Whether you care about getting a proper review from the code owner or not is up to you morally. If you don't care and there is a proper alternative reviewer, then you don't necessarily have to wait, but to each their own...

[rriddle]

mlir/include/mlir/IR/Operation.h

194

IMO we shouldn't have an iterator version here as it is easy to get wrong. moveAfter(block.begin()) can easily fail if the block is empty.

[GMNGeoffrey]

In D79640#2058021, @rriddle wrote:

In D79640#2027782, @GMNGeoffrey wrote:

Pardon my phabricator ignorance, but why is the comment here "This revision was not accepted when it landed; it landed in state Needs Review" immediately below "mehdi_amini accepted this revision"? Is that because River is marked as a blocking reviewer? Seems like a fragile system to have one person be the only one who can approve some set of code. What if they go on vacation?

In general if you are touching something and the code owner is on vacation you wait for them to get back. Whether you care about getting a proper review from the code owner or not is up to you morally. If you don't care and there is a proper alternative reviewer, then you don't necessarily have to wait, but to each their own...

Argue with Mehdi. I didn't land this :-P I do think that having a single point of failure for code reviews is a very fragile system though.

mlir/include/mlir/IR/Operation.h

194

Yeah that will fail, although it's covered by the assert and should also be clear and understandable why it would fail. Our uses of moveAfter in IREE both use the operation format though because we're using it for hoisting (insert after the last op that this uses). We have to special case inserting at the beginning of the block already. I think the use case for the iterator form would have to be after various iterator fiddling where someone would need to check whether the block was empty/iterator was block.end() anyway.

mlir/include/mlir/IR/Operation.h

public:
   /// `existingOp` which may be in the same or another block in the same
   /// function.
   void moveBefore(Operation *existingOp);
 
   /// Unlink this operation from its current block and insert it right before
   /// `iterator` in the specified block.
   void moveBefore(Block *block, llvm::iplist<Operation>::iterator iterator);
 
+  /// Unlink this operation from its current block and insert it right after
+  /// `existingOp` which may be in the same or another block in the same
+  /// function.
+  void moveAfter(Operation *existingOp);
+
+  /// Unlink this operation from its current block and insert it right after
+  /// `iterator` in the specified block.
+  void moveAfter(Block *block, llvm::iplist<Operation>::iterator iterator);
+
   /// Given an operation 'other' that is within the same parent block, return
   /// whether the current operation is before 'other' in the operation list
   /// of the parent block.
   /// Note: This function has an average complexity of O(1), but worst case may
   /// take O(N) where N is the number of operations within the parent block.
   bool isBeforeInBlock(Operation *other);
 
   void print(raw_ostream &os, OpPrintingFlags flags = llvm::None);

mlir/lib/IR/Operation.cpp

 /// Unlink this operation from its current basic block and insert it right
 /// before `iterator` in the specified basic block.
 void Operation::moveBefore(Block *block,
                            llvm::iplist<Operation>::iterator iterator) {
   block->getOperations().splice(iterator, getBlock()->getOperations(),
                                 getIterator());
 }
 
+/// Unlink this operation from its current block and insert it right after
+/// `existingOp` which may be in the same or another block in the same function.
+void Operation::moveAfter(Operation *existingOp) {
+  moveAfter(existingOp->getBlock(), existingOp->getIterator());
+}
+
+/// Unlink this operation from its current block and insert it right after
+/// `iterator` in the specified block.
+void Operation::moveAfter(Block *block,
+                          llvm::iplist<Operation>::iterator iterator) {
+  assert(iterator != block->end() && "cannot move after end of block");
+  moveBefore(&*std::next(iterator));
+}
+
 /// This drops all operand uses from this operation, which is an essential
 /// step in breaking cyclic dependences between references when they are to
 /// be deleted.
 void Operation::dropAllReferences() {
   for (auto &op : getOpOperands())
     op.drop();
 
   for (auto &region : getRegions())