[CIR][Lowering] Fixed break/continue lowering for loops #211

gitoleg · 2023-08-04T08:16:13Z

This PR fixes lowering for break/continue in loops.
The idea is to replace cir.yield break and cir.yield continue with the branch operations to the corresponding blocks. Note, that we need to ignore nesting loops and don't touch break in switch operations. Also, yield from if need to be considered only when it's not the loop yield and continue in switch is ignored since it's processed in the loops lowering.

Fixes #160

…m#160

bcardosolopes

This is pretty cool, thanks for fixing this issue! Added a few comments inline.

@sitio-couto can you also take a look?

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

clang/test/CIR/Lowering/loops-with-continue.cir

sitio-couto

@gitoleg great implementation! Thanks.

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

clang/test/CIR/Lowering/loops-with-break.cir

sitio-couto

@gitoleg great! Thanks.

sitio-couto · 2023-08-09T19:08:41Z

@gitoleg could you squash the commits in this PR and rebase it on main so that we can land it?

bcardosolopes · 2023-08-09T19:18:03Z

@sitio-couto can you try "Squash and merge"? Should do the job!

This PR fixes lowering for `break/continue` in loops. The idea is to replace `cir.yield break` and `cir.yield continue` with the branch operations to the corresponding blocks. Note, that we need to ignore nesting loops and don't touch `break` in switch operations. Also, `yield` from `if` need to be considered only when it's not the loop `yield` and `continue` in switch is ignored since it's processed in the loops lowering. Fixes #160

This PR fixes a couple of corner cases connected with the `YieldOp` lowering in loops. Previously, in #211 we introduced `lowerNestedBreakContinue` but we didn't check that `YieldOp` may belong to the same region, i.e. it is not nested, e.g. ``` while(1) { break; } ``` Hence the error `op already replaced`. Next, we fix `yield` lowering for `ifOp` and `switchOp` but didn't cover `scopeOp`, and the same error occurred. This PR fixes this as well. I added two tests - with no checks actually, just to make sure no more crashes happen. fixes #324

This PR fixes lowering for `break/continue` in loops. The idea is to replace `cir.yield break` and `cir.yield continue` with the branch operations to the corresponding blocks. Note, that we need to ignore nesting loops and don't touch `break` in switch operations. Also, `yield` from `if` need to be considered only when it's not the loop `yield` and `continue` in switch is ignored since it's processed in the loops lowering. Fixes #160

This PR fixes a couple of corner cases connected with the `YieldOp` lowering in loops. Previously, in #211 we introduced `lowerNestedBreakContinue` but we didn't check that `YieldOp` may belong to the same region, i.e. it is not nested, e.g. ``` while(1) { break; } ``` Hence the error `op already replaced`. Next, we fix `yield` lowering for `ifOp` and `switchOp` but didn't cover `scopeOp`, and the same error occurred. This PR fixes this as well. I added two tests - with no checks actually, just to make sure no more crashes happen. fixes #324

This PR fixes lowering for `break/continue` in loops. The idea is to replace `cir.yield break` and `cir.yield continue` with the branch operations to the corresponding blocks. Note, that we need to ignore nesting loops and don't touch `break` in switch operations. Also, `yield` from `if` need to be considered only when it's not the loop `yield` and `continue` in switch is ignored since it's processed in the loops lowering. Fixes #160

This PR fixes a couple of corner cases connected with the `YieldOp` lowering in loops. Previously, in #211 we introduced `lowerNestedBreakContinue` but we didn't check that `YieldOp` may belong to the same region, i.e. it is not nested, e.g. ``` while(1) { break; } ``` Hence the error `op already replaced`. Next, we fix `yield` lowering for `ifOp` and `switchOp` but didn't cover `scopeOp`, and the same error occurred. This PR fixes this as well. I added two tests - with no checks actually, just to make sure no more crashes happen. fixes #324

This PR fixes lowering for `break/continue` in loops. The idea is to replace `cir.yield break` and `cir.yield continue` with the branch operations to the corresponding blocks. Note, that we need to ignore nesting loops and don't touch `break` in switch operations. Also, `yield` from `if` need to be considered only when it's not the loop `yield` and `continue` in switch is ignored since it's processed in the loops lowering. Fixes #160

This PR fixes a couple of corner cases connected with the `YieldOp` lowering in loops. Previously, in #211 we introduced `lowerNestedBreakContinue` but we didn't check that `YieldOp` may belong to the same region, i.e. it is not nested, e.g. ``` while(1) { break; } ``` Hence the error `op already replaced`. Next, we fix `yield` lowering for `ifOp` and `switchOp` but didn't cover `scopeOp`, and the same error occurred. This PR fixes this as well. I added two tests - with no checks actually, just to make sure no more crashes happen. fixes #324

This PR fixes lowering for `break/continue` in loops. The idea is to replace `cir.yield break` and `cir.yield continue` with the branch operations to the corresponding blocks. Note, that we need to ignore nesting loops and don't touch `break` in switch operations. Also, `yield` from `if` need to be considered only when it's not the loop `yield` and `continue` in switch is ignored since it's processed in the loops lowering. Fixes llvm#160

) This PR fixes a couple of corner cases connected with the `YieldOp` lowering in loops. Previously, in llvm#211 we introduced `lowerNestedBreakContinue` but we didn't check that `YieldOp` may belong to the same region, i.e. it is not nested, e.g. ``` while(1) { break; } ``` Hence the error `op already replaced`. Next, we fix `yield` lowering for `ifOp` and `switchOp` but didn't cover `scopeOp`, and the same error occurred. This PR fixes this as well. I added two tests - with no checks actually, just to make sure no more crashes happen. fixes llvm#324