Page MenuHomePhabricator

Add Operation::moveAfter
ClosedPublic

Authored by GMNGeoffrey on Fri, May 8, 11:21 AM.

Details

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

Diff Detail

Event Timeline

GMNGeoffrey created this revision.Fri, May 8, 11:21 AM
Herald added a project: Restricted Project. · View Herald Transcript
GMNGeoffrey marked an inline comment as done.
GMNGeoffrey added inline comments.
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 edited the summary of this revision. (Show Details)Fri, May 8, 11:51 AM
mehdi_amini accepted this revision.Fri, May 8, 1:14 PM

Mehdi, can you land this?

This revision was not accepted when it landed; it landed in state Needs Review.Fri, May 8, 3:36 PM
Closed by commit rG2280cb880d2f: Add Operation::moveAfter (authored by GMNGeoffrey, committed by mehdi_amini). · Explain Why
This revision was automatically updated to reflect the committed changes.

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?

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.

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?

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

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

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 added inline comments.Wed, May 27, 12:09 PM
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 marked an inline comment as done.Wed, May 27, 12:55 PM

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.