[mlir][EmitC] Fix evaluation order of expressions

[simon-camp]

Expressions with the same precedence were not parenthesized and therefore were possibly evaluated in the wrong order depending on the shape of the expression tree.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Simon Camphausen (simon-camp)

Changes

Expressions with the same precedence were not paranthesized and therefore were possibly evaluated in the wrong order depending on the shape of the expression tree.

---

Full diff: https://github.com/llvm/llvm-project/pull/93549.diff

2 Files Affected:

• (modified) mlir/lib/Target/Cpp/TranslateToCpp.cpp (+5-1)
• (modified) mlir/test/Target/Cpp/expressions.mlir (+17)

diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index 7db7163bac4ab..3e7cd30853a98 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -1316,7 +1316,11 @@ LogicalResult CppEmitter::emitOperand(Value value) {
     FailureOr<int> precedence = getOperatorPrecedence(def);
     if (failed(precedence))
       return failure();
-    bool encloseInParenthesis = precedence.value() < getExpressionPrecedence();
+
+    // Expressions with the same precedence need to be paranthesized, as
+    // they might be evaluated in the wrong order depending on the shape of the
+    // expression tree.
+    bool encloseInParenthesis = precedence.value() <= getExpressionPrecedence();
     if (encloseInParenthesis) {
       os << "(";
       pushExpressionPrecedence(lowestPrecedence());
diff --git a/mlir/test/Target/Cpp/expressions.mlir b/mlir/test/Target/Cpp/expressions.mlir
index 2eda58902cb1d..8a3bac3233a93 100644
--- a/mlir/test/Target/Cpp/expressions.mlir
+++ b/mlir/test/Target/Cpp/expressions.mlir
@@ -83,6 +83,23 @@ func.func @paranthesis_for_low_precedence(%arg0: i32, %arg1: i32, %arg2: i32) ->
   return %e : f32
 }
 
+// CPP-DEFAULT:      int32_t paranthesis_for_same_precedence(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]]) {
+// CPP-DEFAULT-NEXT:   return [[VAL_3]] / ([[VAL_1]] * [[VAL_2]]);
+// CPP-DEFAULT-NEXT: }
+
+// CPP-DECLTOP:      int32_t paranthesis_for_same_precedence(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]]) {
+// CPP-DECLTOP-NEXT:   return [[VAL_3]] / ([[VAL_1]] * [[VAL_2]]);
+// CPP-DECLTOP-NEXT: }
+func.func @paranthesis_for_same_precedence(%arg0: i32, %arg1: i32, %arg2: i32) -> i32 {
+  %e = emitc.expression : i32 {
+      %0 = emitc.mul %arg0, %arg1 : (i32, i32) -> i32
+      %1 = emitc.div %arg2, %0 : (i32, i32) -> i32
+      emitc.yield %1 : i32
+    }
+
+  return %e : i32
+}
+
 // CPP-DEFAULT:      int32_t multiple_uses(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]], int32_t [[VAL_4:v[0-9]+]]) {
 // CPP-DEFAULT-NEXT:   bool [[VAL_5:v[0-9]+]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_4]] < [[VAL_2]];
 // CPP-DEFAULT-NEXT:   int32_t [[VAL_6:v[0-9]+]];

[llvmbot]

@llvm/pr-subscribers-mlir-emitc

Author: Simon Camphausen (simon-camp)

Changes

Expressions with the same precedence were not paranthesized and therefore were possibly evaluated in the wrong order depending on the shape of the expression tree.

---

Full diff: https://github.com/llvm/llvm-project/pull/93549.diff

2 Files Affected:

• (modified) mlir/lib/Target/Cpp/TranslateToCpp.cpp (+5-1)
• (modified) mlir/test/Target/Cpp/expressions.mlir (+17)

diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index 7db7163bac4ab..3e7cd30853a98 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -1316,7 +1316,11 @@ LogicalResult CppEmitter::emitOperand(Value value) {
     FailureOr<int> precedence = getOperatorPrecedence(def);
     if (failed(precedence))
       return failure();
-    bool encloseInParenthesis = precedence.value() < getExpressionPrecedence();
+
+    // Expressions with the same precedence need to be paranthesized, as
+    // they might be evaluated in the wrong order depending on the shape of the
+    // expression tree.
+    bool encloseInParenthesis = precedence.value() <= getExpressionPrecedence();
     if (encloseInParenthesis) {
       os << "(";
       pushExpressionPrecedence(lowestPrecedence());
diff --git a/mlir/test/Target/Cpp/expressions.mlir b/mlir/test/Target/Cpp/expressions.mlir
index 2eda58902cb1d..8a3bac3233a93 100644
--- a/mlir/test/Target/Cpp/expressions.mlir
+++ b/mlir/test/Target/Cpp/expressions.mlir
@@ -83,6 +83,23 @@ func.func @paranthesis_for_low_precedence(%arg0: i32, %arg1: i32, %arg2: i32) ->
   return %e : f32
 }
 
+// CPP-DEFAULT:      int32_t paranthesis_for_same_precedence(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]]) {
+// CPP-DEFAULT-NEXT:   return [[VAL_3]] / ([[VAL_1]] * [[VAL_2]]);
+// CPP-DEFAULT-NEXT: }
+
+// CPP-DECLTOP:      int32_t paranthesis_for_same_precedence(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]]) {
+// CPP-DECLTOP-NEXT:   return [[VAL_3]] / ([[VAL_1]] * [[VAL_2]]);
+// CPP-DECLTOP-NEXT: }
+func.func @paranthesis_for_same_precedence(%arg0: i32, %arg1: i32, %arg2: i32) -> i32 {
+  %e = emitc.expression : i32 {
+      %0 = emitc.mul %arg0, %arg1 : (i32, i32) -> i32
+      %1 = emitc.div %arg2, %0 : (i32, i32) -> i32
+      emitc.yield %1 : i32
+    }
+
+  return %e : i32
+}
+
 // CPP-DEFAULT:      int32_t multiple_uses(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]], int32_t [[VAL_4:v[0-9]+]]) {
 // CPP-DEFAULT-NEXT:   bool [[VAL_5:v[0-9]+]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_4]] < [[VAL_2]];
 // CPP-DEFAULT-NEXT:   int32_t [[VAL_6:v[0-9]+]];

[mgehre-amd]

Thanks for the fix! Is this intended for fix #93470?

[aniragil]

Thanks for fixing this @simon-camp!
IINM we can avoid parentheses if not required by associativity, i.e. the expression (a * b) / d can be emitted as a * b / d due to the operator's left associativity, but best to first fix the bug and later relax if desired.

[simon-camp]

Thanks for the fix! Is this intended for fix #93470?

This just fixes the issue in the comment I added to the issue. The original errors are unaffected by this change. I have an idea for a simple fix that I can send for review later today. I'd just have to evaluate how many unneeded parentheses this would produce.

[cferry-AMD]

Thanks for noticing there were actually two issues -- and for taking care of them! Do you want to add the fix you thought of for the other issue in this PR, or do you want to make another PR for it?

[simon-camp]

I'll send another PR for the original issue as there may be need for some discussion.