[mlir][tosa] Interpret boolean values correctly

[CoTinker]

Previously the ClampOp::verify would sign extend boolean values, leading "true" to be casted to a value of -1 instead of 1. This PR ensures i1 values are zero extended, since i1 is used as a boolean value in TOSA.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-tosa

Author: Longsheng Mou (CoTinker)

Changes

According to TOSA spec, clamp op not support boolean type, and the verification and lowering of clamp with boolean type are incorrect. This PR disallow boolean as element type for tosa.clamp to avoid mistake. Fixes #130016.

---

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

2 Files Affected:

• (modified) mlir/lib/Dialect/Tosa/IR/TosaOps.cpp (+3)
• (modified) mlir/test/Dialect/Tosa/invalid.mlir (+9)

diff --git a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
index f0ff430bae882..d3a0b290c0610 100644
--- a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
+++ b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
@@ -928,6 +928,9 @@ LogicalResult tosa::ClampOp::verify() {
   if (inputETy != outputETy)
     return emitOpError("input/output element types are incompatible.");
 
+  if (inputETy.isInteger(1))
+    return emitOpError("does not support boolean element types.");
+
   auto maxValAttr = getMaxValAttr();
   auto minValAttr = getMinValAttr();
 
diff --git a/mlir/test/Dialect/Tosa/invalid.mlir b/mlir/test/Dialect/Tosa/invalid.mlir
index 5a424c41775c9..1f1e4159414f4 100644
--- a/mlir/test/Dialect/Tosa/invalid.mlir
+++ b/mlir/test/Dialect/Tosa/invalid.mlir
@@ -750,6 +750,15 @@ func.func @test_mismatch_in_out_shape_clamp(%arg0: tensor<13x21x3xf32>) -> tenso
 
 // -----
 
+// CHECK-LABEL: test_unsupported_boolean_type_clamp
+func.func @test_unsupported_boolean_type_clamp(%arg0: tensor<13x21x3xi1>) -> tensor<13x21x3xi1> {
+  // expected-error@+1 {{'tosa.clamp' op does not support boolean element types}}
+  %0 = tosa.clamp %arg0 {min_val = false, max_val = true} : (tensor<13x21x3xi1>) -> tensor<13x21x3xi1>
+  return %0 : tensor<13x21x3xi1>
+}
+
+// -----
+
 // CHECK-LABEL: test_mismatch_in_out_data_type_erf
 func.func @test_mismatch_in_out_data_type_erf(%arg0: tensor<13x21x3xf32>) -> tensor<13x21x3xf16> {
   // expected-error@+1 {{'tosa.erf' op requires the same element type for all operands and results}}

[psunn]

LGTM, thank you for resolving this!

[lhutton1]

Thanks for the fix. While the specification doesn't allow clamp on boolean types, the dialect is designed to be more flexible than the specification. Conformance to the specification can be checked using --tosa-validate. Out of curiosity, would the described issue linked above be fixed if we updated the clamp folder to support boolean types? I'm wondering if we should support it in the folder, rather than restrict construction

I agree that it seems more reasonable to enforce the restriction on using boolean as the element type for Clamp during the validation pass, but the appropriate place to implement this still requires further discussion.

[CoTinker]

@lhutton1，Could you please take a look at the new patch.

[lhutton1]

Thanks for the fix, though I wonder if we should remove the "Fixes " line before merging, since I'm not sure it's entirely fixed yet

[CoTinker]

Maybe remove it better.