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

[CIR][Lowering] Fix BrCond Lowering #819

Merged
merged 13 commits into from
Sep 10, 2024
Merged

Conversation

bruteforceboy
Copy link
Contributor

This PR fixes the lowering for BrCond.

Consider the following code snippet:

#include <stdbool.h>

bool test() {
  bool x = false;
  if (x) 
    return x;
  return x;
} 

Emitting the CIR to tmp.cir using -fclangir-mem2reg produces the following CIR (truncated):

!s32i = !cir.int<s, 32>
#fn_attr = #cir<extra({inline = #cir.inline<no>, nothrow = #cir.nothrow, optnone = #cir.optnone})>
module { cir.func no_proto  @test() -> !cir.bool extra(#fn_attr) {
    %0 = cir.const #cir.int<0> : !s32i 
    %1 = cir.cast(int_to_bool, %0 : !s32i), !cir.bool 
    cir.br ^bb1 
  ^bb1:  // pred: ^bb0
    cir.brcond %1 ^bb2, ^bb3 
  ^bb2:  // pred: ^bb1
    cir.return %1 : !cir.bool 
  ^bb3:  // pred: ^bb1
    cir.br ^bb4 
  ^bb4:  // pred: ^bb3
    cir.return %1 : !cir.bool 
  } 
}

Lowering the CIR to LLVM using cir-opt tmp.cir -cir-to-llvm fails with:

tmp.cir:5:10: error: failed to legalize operation 'llvm.zext' marked as erased

The CIR cast %1 = cir.cast(int_to_bool, %0 : !s32i) is lowered to a CIR comparison with zero, which is then lowered to an LLVM::ICmpOp and LLVM::ZExtOp.

In the BrCond lowering, the zext is deleted when zext->use_empty(), but during this phase the lowering for the CIR above is not complete yet, because the zext will still have usage(s) later.

The current check for when the zext is deleted is error-prone and can be improved.

To fix this, in addition to checking that the use of the zext is empty, an additional check that the defining operation for the BrCond in the CIR (the cast operation in this case) is used exactly once is added.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this bug and thanks @smeenai for reviewing! Some changes needed but this almost good to go.

clang/test/CIR/Lowering/brcond.c Outdated Show resolved Hide resolved
clang/test/CIR/Lowering/brcond.c Outdated Show resolved Hide resolved
clang/test/CIR/Lowering/brcond.cir Outdated Show resolved Hide resolved
clang/test/CIR/Lowering/brcond.cir Outdated Show resolved Hide resolved
clang/test/CIR/Lowering/brcond.c Outdated Show resolved Hide resolved
@bcardosolopes bcardosolopes merged commit 53bdfb5 into llvm:main Sep 10, 2024
6 checks passed
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
This PR fixes the lowering for BrCond. 

Consider the following code snippet: 
```
#include <stdbool.h>

bool test() {
  bool x = false;
  if (x) 
    return x;
  return x;
} 
```
Emitting the CIR to `tmp.cir` using `-fclangir-mem2reg` produces the
following CIR (truncated):
```
!s32i = !cir.int<s, 32>
#fn_attr = #cir<extra({inline = #cir.inline<no>, nothrow = #cir.nothrow, optnone = #cir.optnone})>
module { cir.func no_proto  @test() -> !cir.bool extra(#fn_attr) {
    %0 = cir.const #cir.int<0> : !s32i 
    %1 = cir.cast(int_to_bool, %0 : !s32i), !cir.bool 
    cir.br ^bb1 
  ^bb1:  // pred: ^bb0
    cir.brcond %1 ^bb2, ^bb3 
  ^bb2:  // pred: ^bb1
    cir.return %1 : !cir.bool 
  ^bb3:  // pred: ^bb1
    cir.br ^bb4 
  ^bb4:  // pred: ^bb3
    cir.return %1 : !cir.bool 
  } 
}
```
Lowering the CIR to LLVM using `cir-opt tmp.cir -cir-to-llvm` fails
with:
```
tmp.cir:5:10: error: failed to legalize operation 'llvm.zext' marked as erased
```

The CIR cast `%1 = cir.cast(int_to_bool, %0 : !s32i)` is lowered to a
CIR comparison with zero, which is then lowered to an `LLVM::ICmpOp` and
`LLVM::ZExtOp`.

In the BrCond lowering, the zext is deleted when `zext->use_empty()`,
but during this phase the lowering for the CIR above is not complete
yet, because the zext will still have usage(s) later.

The current check for when the zext is deleted is error-prone and can be
improved.

To fix this, in addition to checking that the use of the zext is empty,
an additional check that the defining operation for the BrCond in the
CIR (the cast operation in this case) is used exactly once is added.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
This PR fixes the lowering for BrCond. 

Consider the following code snippet: 
```
#include <stdbool.h>

bool test() {
  bool x = false;
  if (x) 
    return x;
  return x;
} 
```
Emitting the CIR to `tmp.cir` using `-fclangir-mem2reg` produces the
following CIR (truncated):
```
!s32i = !cir.int<s, 32>
#fn_attr = #cir<extra({inline = #cir.inline<no>, nothrow = #cir.nothrow, optnone = #cir.optnone})>
module { cir.func no_proto  @test() -> !cir.bool extra(#fn_attr) {
    %0 = cir.const #cir.int<0> : !s32i 
    %1 = cir.cast(int_to_bool, %0 : !s32i), !cir.bool 
    cir.br ^bb1 
  ^bb1:  // pred: ^bb0
    cir.brcond %1 ^bb2, ^bb3 
  ^bb2:  // pred: ^bb1
    cir.return %1 : !cir.bool 
  ^bb3:  // pred: ^bb1
    cir.br ^bb4 
  ^bb4:  // pred: ^bb3
    cir.return %1 : !cir.bool 
  } 
}
```
Lowering the CIR to LLVM using `cir-opt tmp.cir -cir-to-llvm` fails
with:
```
tmp.cir:5:10: error: failed to legalize operation 'llvm.zext' marked as erased
```

The CIR cast `%1 = cir.cast(int_to_bool, %0 : !s32i)` is lowered to a
CIR comparison with zero, which is then lowered to an `LLVM::ICmpOp` and
`LLVM::ZExtOp`.

In the BrCond lowering, the zext is deleted when `zext->use_empty()`,
but during this phase the lowering for the CIR above is not complete
yet, because the zext will still have usage(s) later.

The current check for when the zext is deleted is error-prone and can be
improved.

To fix this, in addition to checking that the use of the zext is empty,
an additional check that the defining operation for the BrCond in the
CIR (the cast operation in this case) is used exactly once is added.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
This PR fixes the lowering for BrCond. 

Consider the following code snippet: 
```
#include <stdbool.h>

bool test() {
  bool x = false;
  if (x) 
    return x;
  return x;
} 
```
Emitting the CIR to `tmp.cir` using `-fclangir-mem2reg` produces the
following CIR (truncated):
```
!s32i = !cir.int<s, 32>
#fn_attr = #cir<extra({inline = #cir.inline<no>, nothrow = #cir.nothrow, optnone = #cir.optnone})>
module { cir.func no_proto  @test() -> !cir.bool extra(#fn_attr) {
    %0 = cir.const #cir.int<0> : !s32i 
    %1 = cir.cast(int_to_bool, %0 : !s32i), !cir.bool 
    cir.br ^bb1 
  ^bb1:  // pred: ^bb0
    cir.brcond %1 ^bb2, ^bb3 
  ^bb2:  // pred: ^bb1
    cir.return %1 : !cir.bool 
  ^bb3:  // pred: ^bb1
    cir.br ^bb4 
  ^bb4:  // pred: ^bb3
    cir.return %1 : !cir.bool 
  } 
}
```
Lowering the CIR to LLVM using `cir-opt tmp.cir -cir-to-llvm` fails
with:
```
tmp.cir:5:10: error: failed to legalize operation 'llvm.zext' marked as erased
```

The CIR cast `%1 = cir.cast(int_to_bool, %0 : !s32i)` is lowered to a
CIR comparison with zero, which is then lowered to an `LLVM::ICmpOp` and
`LLVM::ZExtOp`.

In the BrCond lowering, the zext is deleted when `zext->use_empty()`,
but during this phase the lowering for the CIR above is not complete
yet, because the zext will still have usage(s) later.

The current check for when the zext is deleted is error-prone and can be
improved.

To fix this, in addition to checking that the use of the zext is empty,
an additional check that the defining operation for the BrCond in the
CIR (the cast operation in this case) is used exactly once is added.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
This PR fixes the lowering for BrCond. 

Consider the following code snippet: 
```
#include <stdbool.h>

bool test() {
  bool x = false;
  if (x) 
    return x;
  return x;
} 
```
Emitting the CIR to `tmp.cir` using `-fclangir-mem2reg` produces the
following CIR (truncated):
```
!s32i = !cir.int<s, 32>
#fn_attr = #cir<extra({inline = #cir.inline<no>, nothrow = #cir.nothrow, optnone = #cir.optnone})>
module { cir.func no_proto  @test() -> !cir.bool extra(#fn_attr) {
    %0 = cir.const #cir.int<0> : !s32i 
    %1 = cir.cast(int_to_bool, %0 : !s32i), !cir.bool 
    cir.br ^bb1 
  ^bb1:  // pred: ^bb0
    cir.brcond %1 ^bb2, ^bb3 
  ^bb2:  // pred: ^bb1
    cir.return %1 : !cir.bool 
  ^bb3:  // pred: ^bb1
    cir.br ^bb4 
  ^bb4:  // pred: ^bb3
    cir.return %1 : !cir.bool 
  } 
}
```
Lowering the CIR to LLVM using `cir-opt tmp.cir -cir-to-llvm` fails
with:
```
tmp.cir:5:10: error: failed to legalize operation 'llvm.zext' marked as erased
```

The CIR cast `%1 = cir.cast(int_to_bool, %0 : !s32i)` is lowered to a
CIR comparison with zero, which is then lowered to an `LLVM::ICmpOp` and
`LLVM::ZExtOp`.

In the BrCond lowering, the zext is deleted when `zext->use_empty()`,
but during this phase the lowering for the CIR above is not complete
yet, because the zext will still have usage(s) later.

The current check for when the zext is deleted is error-prone and can be
improved.

To fix this, in addition to checking that the use of the zext is empty,
an additional check that the defining operation for the BrCond in the
CIR (the cast operation in this case) is used exactly once is added.
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 this pull request may close these issues.

3 participants