-
Notifications
You must be signed in to change notification settings - Fork 146
[CIR][ThroughMLIR] Support lowering cir.do to scf.while #695
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
Conversation
@ShivaChen @GaoXiangYa @Kritoooo can you please review this? |
rewriter->cloneRegionBefore(DoOp.getCond(), &scfWhileOp.getAfter().back()); | ||
rewriter->eraseBlock(&scfWhileOp.getAfter().back()); | ||
|
||
rewriter->inlineBlockBefore(&scfWhileOp->getRegion(1).back(), | ||
&scfWhileOp->getRegion(0).back(), | ||
scfWhileOp->getRegion(0).back().end()); |
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.
It looks like you placed the condition block at the end of the After region and then moved it to the Before region. Why not complete this in one step by placing the condition block directly at the end of the Before region? Of course, the effect is almost the same.
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.
Sorry, I tried the way you said but got wrong info before. Maybe I need to learn MLIR API deeply.
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.
OK, I'm not very familiar with the SCF dialect, but it seems that a simple do-while loop should not pose any problems (though nesting other control flow statements might cause issues). Maybe we can see what other reviewers think.
// FIXME: can not support nested do-while | ||
|
||
auto scfWhileOp = rewriter->create<mlir::scf::WhileOp>( | ||
DoOp.getLoc(), DoOp->getResultTypes(), adaptor.getOperands()); |
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.
Could we use region builder and mergeBlocks as https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/SCF/Transforms/WrapInZeroTripCheck.cpp#L101?
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.
Nice! I will try that when free.
We just did a rebase against upstream, this PR needs to be updated to account for that, sorry for the churn! |
I had modified this pr. |
This pr should be closed, I have dealt with this problems in #756 @bcardosolopes |
Thanks for clarifying @GaoXiangYa |
This PR implements a simple do-while statement lowering cir.do to scf.while.