[PATCH] D79640: Add Operation::moveAfter

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey marked an inline comment as done.
GMNGeoffrey added a comment.

In D79640#2058021 <https://reviews.llvm.org/D79640#2058021>, @rriddle wrote:

> In D79640#2027782 <https://reviews.llvm.org/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.



================
Comment at: mlir/include/mlir/IR/Operation.h:194
+  /// 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);
----------------
rriddle wrote:
> 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. 
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79640/new/

https://reviews.llvm.org/D79640