[MLIR][EmitC][cf] Bugfix: correctly inline emitc.expression op in the emitted if condition of a cf.cond_br #128958
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… emitted if condition of a cf.cond_br
Running `mlir-translate -mlir-to-cpp -declare-variables-at-top input.mlir` with `input.mlir` as
```
module {
emitc.func @f(%0 : i32, %1 : i32) {
%2 = expression : i1 {
%3 = cmp lt, %0, %1 : (i32, i32) -> i1
yield %3 : i1
}
cf.cond_br %2, ^bb1, ^bb1
^bb1: // 2 preds: ^bb0, ^bb0
return
}
}
```
doesn't inline the expression %2 and generates a use of an undeclared variable in the generated if:
```
void f(int32_t v1, int32_t v2) {
if (v3) {
goto label2;
} else {
goto label2;
}
label2:
return;
}
```
|
@llvm/pr-subscribers-mlir Author: None (gdehame) Changes[MLIR][EmitC][cf] Bugfix: correctly inline emitc.expression op in the emitted if condition of a cf.cond_br Full diff: https://github.com/llvm/llvm-project/pull/128958.diff 2 Files Affected:
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index abff252575eb0..b00820ffc542b 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -613,8 +613,10 @@ static LogicalResult printOperation(CppEmitter &emitter,
Block &trueSuccessor = *condBranchOp.getTrueDest();
Block &falseSuccessor = *condBranchOp.getFalseDest();
- os << "if (" << emitter.getOrCreateName(condBranchOp.getCondition())
- << ") {\n";
+ os << "if (";
+ if (failed(emitter.emitOperand(condBranchOp.getCondition())))
+ return failure();
+ os << ") {\n";
os.indent();
diff --git a/mlir/test/Target/Cpp/control_flow.mlir b/mlir/test/Target/Cpp/control_flow.mlir
index 436543f7ace95..101b30c2521c9 100644
--- a/mlir/test/Target/Cpp/control_flow.mlir
+++ b/mlir/test/Target/Cpp/control_flow.mlir
@@ -68,3 +68,22 @@ func.func @block_labels1() {
// CPP-DECLTOP-NEXT: label2:
// CPP-DECLTOP-NEXT: return;
// CPP-DECLTOP-NEXT: }
+
+emitc.func @expression_inlining(%0 : i32, %1 : i32) {
+ %2 = expression : i1 {
+ %3 = cmp lt, %0, %1 : (i32, i32) -> i1
+ yield %3 : i1
+ }
+ cf.cond_br %2, ^bb1, ^bb1
+ ^bb1: // 2 preds: ^bb0, ^bb0
+ return
+}
+// CPP-DECLTOP: void expression_inlining(int32_t [[v1:v.*]], int32_t [[v2:v.*]]) {
+// CPP-DECLTOP-NEXT: if ([[v1]] < [[v2]]) {
+// CPP-DECLTOP-NEXT: goto label2;
+// CPP-DECLTOP-NEXT: } else {
+// CPP-DECLTOP-NEXT: goto label2;
+// CPP-DECLTOP-NEXT: }
+// CPP-DECLTOP-NEXT: label2:
+// CPP-DECLTOP-NEXT: return;
+// CPP-DECLTOP-NEXT: }
|
|
@llvm/pr-subscribers-mlir-emitc Author: None (gdehame) Changes[MLIR][EmitC][cf] Bugfix: correctly inline emitc.expression op in the emitted if condition of a cf.cond_br Full diff: https://github.com/llvm/llvm-project/pull/128958.diff 2 Files Affected:
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index abff252575eb0..b00820ffc542b 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -613,8 +613,10 @@ static LogicalResult printOperation(CppEmitter &emitter,
Block &trueSuccessor = *condBranchOp.getTrueDest();
Block &falseSuccessor = *condBranchOp.getFalseDest();
- os << "if (" << emitter.getOrCreateName(condBranchOp.getCondition())
- << ") {\n";
+ os << "if (";
+ if (failed(emitter.emitOperand(condBranchOp.getCondition())))
+ return failure();
+ os << ") {\n";
os.indent();
diff --git a/mlir/test/Target/Cpp/control_flow.mlir b/mlir/test/Target/Cpp/control_flow.mlir
index 436543f7ace95..101b30c2521c9 100644
--- a/mlir/test/Target/Cpp/control_flow.mlir
+++ b/mlir/test/Target/Cpp/control_flow.mlir
@@ -68,3 +68,22 @@ func.func @block_labels1() {
// CPP-DECLTOP-NEXT: label2:
// CPP-DECLTOP-NEXT: return;
// CPP-DECLTOP-NEXT: }
+
+emitc.func @expression_inlining(%0 : i32, %1 : i32) {
+ %2 = expression : i1 {
+ %3 = cmp lt, %0, %1 : (i32, i32) -> i1
+ yield %3 : i1
+ }
+ cf.cond_br %2, ^bb1, ^bb1
+ ^bb1: // 2 preds: ^bb0, ^bb0
+ return
+}
+// CPP-DECLTOP: void expression_inlining(int32_t [[v1:v.*]], int32_t [[v2:v.*]]) {
+// CPP-DECLTOP-NEXT: if ([[v1]] < [[v2]]) {
+// CPP-DECLTOP-NEXT: goto label2;
+// CPP-DECLTOP-NEXT: } else {
+// CPP-DECLTOP-NEXT: goto label2;
+// CPP-DECLTOP-NEXT: }
+// CPP-DECLTOP-NEXT: label2:
+// CPP-DECLTOP-NEXT: return;
+// CPP-DECLTOP-NEXT: }
|
marbre
reviewed
Feb 27, 2025
There was a problem hiding this comment.
Thanks for the fix! Will try to find time to take a closer look later. That said, you seem to have an anonymous email address set in your GH settings. Please see https://llvm.org/docs/DeveloperPolicy.html#github-email-address in order to allow us to land your patch once it got approval.
simon-camp
approved these changes
Feb 27, 2025
|
Seems something with the PR description went wrong (it includes the PR title and the body seems to be a code quote). As this will be the body of the commit message we should fix that prior to merging. Do you want me to take that or would you like to edit it yourself @gdehame? |
|
I edited the comment to remove the title from it and replace the example of misbehave with a description of the problem |
marbre
approved these changes
Feb 28, 2025
|
Thanks for the fix! |
cheezeburglar
pushed a commit
to cheezeburglar/llvm-project
that referenced
this pull request
Feb 28, 2025
… emitted if condition of a cf.cond_br (llvm#128958) emitc.expression ops are expected to be inlined in the if condition in the lowering of cf.cond_br if this is their only use but they weren't inlined. Instead, a use of the variable corresponding to the expression result was generated but with no declaration/definition.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
emitc.expression ops are expected to be inlined in the if condition in the lowering of cf.cond_br if this is their only use but they weren't inlined.
Instead, a use of the variable corresponding to the expression result was generated but with no declaration/definition.