-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[mlir][IR] Add rewriter API for moving operations #78988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -366,3 +366,31 @@ void RewriterBase::cloneRegionBefore(Region ®ion, Region &parent, | |
void RewriterBase::cloneRegionBefore(Region ®ion, 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear to me that this is enough to implement a "moveOpBefore" safely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you referring to the fact that "moving ops" is not supported by the dialect conversion? The "Moving an op" is a form of "inserting an op". Until now, we used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear to me what operations should be considered "changed" when we move an op. For example sinking:
to
Is the "if" modified? The "for"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We actually have the same issue with other kinds of IR changes:
My conclusion was that we should not trigger a I think we should trigger Another thing that we could consider is giving
I'd say that But it raises the question whether /// 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) {} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK! |
||
} | ||
|
||
void RewriterBase::moveOpAfter(Operation *op, Operation *existingOp) { | ||
moveOpAfter(op, existingOp->getBlock(), existingOp->getIterator()); | ||
} | ||
|
||
void RewriterBase::moveOpAfter(Operation *op, Block *block, | ||
Block::iterator iterator) { | ||
Block *currentBlock = op->getBlock(); | ||
Block::iterator currentIterator = op->getIterator(); | ||
op->moveAfter(block, iterator); | ||
if (listener) | ||
listener->notifyOperationInserted( | ||
op, /*previous=*/InsertPoint(currentBlock, currentIterator)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -193,9 +193,7 @@ struct HoistEligibleOps : public OpRewritePattern<test::OneRegionOp> { | |
return failure(); | ||
if (!toBeHoisted->hasAttr("eligible")) | ||
return failure(); | ||
// Hoisting means removing an op from the enclosing op. I.e., the enclosing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a workaround around the fact that there were no notifications for |
||
// op is modified. | ||
rewriter.modifyOpInPlace(op, [&]() { toBeHoisted->moveBefore(op); }); | ||
rewriter.moveOpBefore(toBeHoisted, op); | ||
return success(); | ||
} | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is similar to what was proposed here (IR listeners), but the callback is fired after the IR was modified.