[CIR][ThroughMLIR] Lower WhileOp with break #1735
Conversation
| } | ||
|
|
||
| // Operations after this BreakOp has to be removed. | ||
| for (mlir::Operation *runner = breakOp->getNextNode(); runner;) { |
There was a problem hiding this comment.
Why you need to remove this? Looks like we should just split the block at this point and create an unrecheable block (which should get later DCE'd by canonicalizer)?
There was a problem hiding this comment.
I don't know whether it's allowed to delete a non-empty loop (as I'm deleting the loop below). I'll change it to splitting the block if it doesn't matter or we can find a better way below.
| } | ||
|
|
||
| // Blocks after this BreakOp also has to be removed. | ||
| for (mlir::Block *block = breakOp->getBlock()->getNextNode(); block;) { |
There was a problem hiding this comment.
This ties back to the other comment
|
|
||
| // We know this BreakOp isn't nested in any IfOp. | ||
| // Therefore, the loop is executed only once. | ||
| // We pull everything out of the loop. |
There was a problem hiding this comment.
Seems like your are optimizing while you are lowering, why isn't this a separate pass?
There was a problem hiding this comment.
I think we need to erase the BreakOp because scf doesn't support it, so we have to rewrite the loop in some way to preserve semantics. Deleting the loop is the most straightforward way I can think of. Could you suggest better ways of rewriting?
There was a problem hiding this comment.
I see your point! This is good for now.
Thinking more about this it seems like these things should actually be handled in a pass before the actual SCF lowering (something like a CoreDialectPrepare kinda thing) - just like we have a CFGFlatten pre LLVM we could have something that massage CIR loops into CIR loops suitable for more direct SCF translation. This is more food for thought for the future, where you will probably have gathered more examples (cir.continue is slightly different but suffers from a similar issue?).
There was a problem hiding this comment.
That's a good idea. I think I'll do the cir.switch handling (which I'm working on now) in a new pass, and move these break/continue lowering to there afterwards.
This only deals with the case in which the
breakis directly under the loop, not nested in anyifs.In this case we simply erase the loop as well as unreachable code after that
break.