[mlir][math] Fix casting mistake in tanh expansion and test

[srcarroll]

Accidentally used sitofp for bool cast instead of uitofp. A correctness test has also been added

[llvmbot]

@llvm/pr-subscribers-mlir-math

@llvm/pr-subscribers-mlir

Author: None (srcarroll)

Changes

Accidentally used sitofp for bool cast instead of uitofp

---

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

3 Files Affected:

• (modified) mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp (+1-1)
• (modified) mlir/test/Dialect/Math/expand-math.mlir (+1-1)
• (modified) mlir/test/mlir-cpu-runner/test-expand-math-approx.mlir (+19)

diff --git a/mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp b/mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
index 1750171b81a10e..fceafcff8490c3 100644
--- a/mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
+++ b/mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
@@ -108,7 +108,7 @@ static LogicalResult convertTanhOp(math::TanhOp op, PatternRewriter &rewriter) {
   // Compute sign(x) = cast<float_type>(x < 0) * (-2) + 1
   Value sign = rewriter.create<arith::CmpFOp>(loc, arith::CmpFPredicate::OLT,
                                               op.getOperand(), zero);
-  sign = rewriter.create<arith::SIToFPOp>(loc, floatType, sign);
+  sign = rewriter.create<arith::UIToFPOp>(loc, floatType, sign);
   sign = rewriter.create<arith::MulFOp>(loc, sign, negTwo);
   sign = rewriter.create<arith::AddFOp>(loc, sign, one);
 
diff --git a/mlir/test/Dialect/Math/expand-math.mlir b/mlir/test/Dialect/Math/expand-math.mlir
index 86ee5c8620472b..6326d3a71874b4 100644
--- a/mlir/test/Dialect/Math/expand-math.mlir
+++ b/mlir/test/Dialect/Math/expand-math.mlir
@@ -9,7 +9,7 @@ func.func @tanh(%arg: f32) -> f32 {
 // CHECK-DAG: %[[ONE:.+]] = arith.constant 1.000000e+00 : f32
 // CHECK-DAG: %[[TWO:.+]] = arith.constant -2.000000e+00 : f32
 // CHECK: %[[VAL0:.+]] = arith.cmpf olt, %arg0, %[[ZERO]] : f32
-// CHECK: %[[VAL1:.+]] = arith.sitofp %[[VAL0]] : i1 to f32
+// CHECK: %[[VAL1:.+]] = arith.uitofp %[[VAL0]] : i1 to f32
 // CHECK: %[[VAL2:.+]] = arith.mulf %[[VAL1]], %[[TWO]] : f32
 // CHECK: %[[SIGN:.+]] = arith.addf %[[VAL2]], %[[ONE]] : f32
 // CHECK: %[[POSX:.+]] = arith.mulf %[[SIGN]], %arg0 : f32
diff --git a/mlir/test/mlir-cpu-runner/test-expand-math-approx.mlir b/mlir/test/mlir-cpu-runner/test-expand-math-approx.mlir
index 541a201c94c586..e2229a392bbf76 100644
--- a/mlir/test/mlir-cpu-runner/test-expand-math-approx.mlir
+++ b/mlir/test/mlir-cpu-runner/test-expand-math-approx.mlir
@@ -683,6 +683,24 @@ func.func @cosh() {
  return
 }
 
+// -------------------------------------------------------------------------- //
+// Tanh.
+// -------------------------------------------------------------------------- //
+
+func.func @tanh_8xf32(%a : vector<8xf32>) {
+  %r = math.tanh %a : vector<8xf32>
+  vector.print %r : vector<8xf32>
+  return
+}
+
+func.func @tanh() {
+  // CHECK: -1, -0.761594, -0.291313, 0, 0.291313, 0.761594, 1, 1
+  %v3 = arith.constant dense<[0xff800000, -1.0, -0.3, 0.0, 0.3, 1.0, 10.0, 0x7f800000]> : vector<8xf32>
+  call @tanh_8xf32(%v3) : (vector<8xf32>) -> ()
+
+ return
+}
+
 func.func @main() {
   call @exp2f() : () -> ()
   call @roundf() : () -> ()
@@ -690,5 +708,6 @@ func.func @main() {
   call @roundeven() : () -> ()
   call @sinh() : () -> ()
   call @cosh() : () -> ()
+  call @tanh() : () -> ()
   return
 }

[srcarroll]

My bad for using the wrong cast. I had this tested in my own pipeline but this cast ended up getting removed so I didn't notice

[makslevental]

Is the original/current actually wrong? arith::CmpFOp returns i1 i.e., signless, which means roughly (in my understanding, which might be mistaken) "up to you", i.e., target (dialect or arch or whatever) dependent. So how can you pick SIToFPOp or UIToFPOp at all here? I mean, maybe the real issue is that there's no arith::IToFP for cases like this.

[srcarroll]

Good point/question. I only knew by observing that sitofp was "wrong" (gave unintentional results) as it gave me -1 for true and 0 for false. Whereas uitofp gives me 1 for true. But honestly not sure if this works as a target agnostic solution.

[srcarroll]

Any suggestions? Should i just revert my original merge until this is sorted?

[makslevental]

Any suggestions? Should i just revert my original merge until this is sorted?

General operating procedure is to get build bots back to green ASAP, i.e., revert or "fix forward". Technically this didn't break build bots I guess but same good faith interpretation applies (since you believe this is a breaking change). I can't approve because I can't manage to unwind right now all of bit patterns so yea revert (but obv don't abandon this one...).

[srcarroll]

ok. reverting #85429 and drafting this for now

[srcarroll]

closing in favor of #85436