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.

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

Lowering of nested break/continue in loops #160

Closed
yugr opened this issue Jul 10, 2023 · 4 comments · Fixed by #211
Closed

Lowering of nested break/continue in loops #160

yugr opened this issue Jul 10, 2023 · 4 comments · Fixed by #211

Comments

@yugr
Copy link
Member

yugr commented Jul 10, 2023

Looks like currently CIRLoopOpLowering does not lower the nested break/continue statements within the lowered loop. For example this program

#include <stdio.h>

int foo(int n) {
  int s = 0, i;
  for (i = 1; i < n; ++i) {
    if (i == 1)
      break;
    ++s;
  }
  return s;
}

int main() {
  int s = foo(10);
  printf("%d\n", s);
  return 0;
}

prints 9 instead of expected 0.

Probably the best way to fix this is to scan body region inside CIRLoopLowering, replacing break/continue yields with BrOps?

@bcardosolopes
Copy link
Member

Thanks for catching this. @sitio-couto seems like you are going to hit this sooner or later!

@yugr
Copy link
Member Author

yugr commented Jul 11, 2023

Probably the best way to fix this is to scan body region inside CIRLoopLowering, replacing break/continue yields with BrOps?

We did this and it seems to work fine. I think we should be able to send PR in few days if you are ok with this approach.

@bcardosolopes
Copy link
Member

We did this and it seems to work fine. I think we should be able to send PR in few days if you are ok with this approach.

PRs are great :)

For full context, ideally we should lower those to a more simple form of CIR before LLVM lowering happens, so we can keep LLVM lowering a bit more simple. However, there are other changes we'd need before that in general and looks like your approach would work for now, so I'm ok with it. Let's just make sure you and @sitio-couto coordinate a bit to prevent dup work (we're also on discord if you'd like to discuss any other aspect).

@sitio-couto
Copy link
Collaborator

Let's just make sure you and @sitio-couto coordinate a bit to prevent dup work

@yugr I haven't reached unstructured loops yet. Feel free to work on this!

sitio-couto pushed a commit that referenced this issue Aug 9, 2023
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
lanza pushed a commit that referenced this issue Oct 27, 2023
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
lanza pushed a commit that referenced this issue Dec 20, 2023
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
lanza pushed a commit that referenced this issue Jan 29, 2024
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
lanza pushed a commit that referenced this issue Mar 23, 2024
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
eZWALT pushed a commit to eZWALT/clangir that referenced this issue Mar 24, 2024
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
eZWALT pushed a commit to eZWALT/clangir that referenced this issue Mar 24, 2024
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
lanza pushed a commit that referenced this issue Apr 29, 2024
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
lanza pushed a commit that referenced this issue Apr 29, 2024
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
eZWALT pushed a commit to eZWALT/clangir that referenced this issue Apr 29, 2024
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
lanza pushed a commit that referenced this issue Apr 29, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants