Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Loop with break or continue as last statement causes assertion failure #324

Closed
dkolsen-pgi opened this issue Nov 20, 2023 · 0 comments · Fixed by #349
Closed

Loop with break or continue as last statement causes assertion failure #324

dkolsen-pgi opened this issue Nov 20, 2023 · 0 comments · Fixed by #349

Comments

@dkolsen-pgi
Copy link
Collaborator

When a for, while, or do/while loop contains a break or continue statement at the top level of the loop's body and the last statement of the block, that triggers an assertion failure during ClangIR lowering to LLVM dialect.

void test(int x) {
  for (int i = 0; i < x; ++i) {
    --x;
    break;
  }
}

clang: .../mlir/lib/Transforms/Utils/DialectConversion.cpp:1372: 
void mlir::detail::ConversionPatternRewriterImpl::notifyOpReplaced(mlir::Operation*, mlir::ValueRange): 
Assertion `!replacements.count(op) && "operation was already replaced"' failed.

If the break or continue is in a nested scope within the loop (which is the normal case in real code), there is no error. If the break or continue statement is not the last statement in the block then there is a different internal error which is covered by #323.
@dkolsen-pgi dkolsen-pgi mentioned this issue Nov 20, 2023
Closed
@gitoleg gitoleg mentioned this issue Dec 7, 2023
Merged
bcardosolopes pushed a commit that referenced this issue Dec 8, 2023
@gitoleg
[CIR][Lowering] Fix loop lowering for top-level break/continue (#349)
350057a 
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
lanza pushed a commit that referenced this issue Dec 20, 2023
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (#349)
6a9391a 
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
lanza pushed a commit that referenced this issue Jan 29, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (#349)
933a0a9 
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
lanza pushed a commit that referenced this issue Mar 23, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (#349)
2475647 
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
eZWALT pushed a commit to eZWALT/clangir that referenced this issue Mar 24, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (llvm#349
b89132d 
)

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
eZWALT pushed a commit to eZWALT/clangir that referenced this issue Mar 24, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (llvm#349
d6c7fd3 
)

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
lanza pushed a commit that referenced this issue Apr 29, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (#349)
a59357e 
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
lanza pushed a commit that referenced this issue Apr 29, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (#349)
c3020ad 
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
eZWALT pushed a commit to eZWALT/clangir that referenced this issue Apr 29, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (llvm#349
ef7c7a2 
)

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
lanza pushed a commit that referenced this issue Apr 29, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (#349)
e647865 
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
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this issue Oct 2, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (llvm#349
4b07109 
)

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
Hugobros3 pushed a commit to shady-gang/clangir that referenced this issue Oct 2, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (llvm#349
2e21e36 
)

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
keryell pushed a commit to keryell/clangir that referenced this issue Oct 19, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (llvm#349
803c8bb 
)

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
lanza pushed a commit that referenced this issue Nov 5, 2024
@gitoleg @lanza
[CIR][Lowering] Fix loop lowering for top-level break/continue (#349)
ee6d21f 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[CIR][Lowering] fixes loop lowering for top-level break/continue gitoleg/clangir
1 participant
@dkolsen-pgi