[mlir][tosa] Fix validation check on controlflow operators #159754
Conversation
|
@llvm/pr-subscribers-mlir Author: Luke Hutton (lhutton1) ChangesPrevioulsy the error_if check for controlflow operators would silently fail on valid controflow operators. This was due to incorrect return logic in the validation function. This commit fixes that logic. Full diff: https://github.com/llvm/llvm-project/pull/159754.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp b/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
index 790bbf77877bc..e6091df367754 100644
--- a/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
+++ b/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
@@ -1257,8 +1257,8 @@ bool checkErrorIfCondIf(Operation *op) {
// tosa.yield %arg4
// }
- return failed(checkIsolatedRegion(op, ifOp.getThenGraph(), "then")) ||
- failed(checkIsolatedRegion(op, ifOp.getElseGraph(), "else"));
+ return succeeded(checkIsolatedRegion(op, ifOp.getThenGraph(), "then")) &&
+ succeeded(checkIsolatedRegion(op, ifOp.getElseGraph(), "else"));
}
bool checkErrorIfWhileLoop(Operation *op) {
@@ -1266,8 +1266,8 @@ bool checkErrorIfWhileLoop(Operation *op) {
if (!whileOp)
return true;
- return failed(checkIsolatedRegion(op, whileOp.getCondGraph(), "cond")) ||
- failed(checkIsolatedRegion(op, whileOp.getBodyGraph(), "body"));
+ return succeeded(checkIsolatedRegion(op, whileOp.getCondGraph(), "cond")) &&
+ succeeded(checkIsolatedRegion(op, whileOp.getBodyGraph(), "body"));
}
bool checkErrorIfScatter(Operation *op) {
diff --git a/mlir/test/Dialect/Tosa/tosa-validation-valid-strict.mlir b/mlir/test/Dialect/Tosa/tosa-validation-valid-strict.mlir
new file mode 100644
index 0000000000000..fe423104359ab
--- /dev/null
+++ b/mlir/test/Dialect/Tosa/tosa-validation-valid-strict.mlir
@@ -0,0 +1,15 @@
+// RUN: mlir-opt %s -split-input-file -verify-diagnostics --tosa-validate="profile=pro_int,pro_fp extension=int16,int4,bf16,fp8e4m3,fp8e5m2,fft,variable,controlflow,doubleround,inexactround strict-op-spec-alignment" | FileCheck %s
+
+// -----
+
+// CHECK-LABEL: test_cond_if
+func.func @test_cond_if(%arg0: tensor<i8>, %arg1: tensor<i8>, %arg2: tensor<i1>) -> tensor<i8> {
+ %0 = tosa.cond_if %arg2 (%arg3 = %arg0, %arg4 = %arg1) : tensor<i1> (tensor<i8>, tensor<i8>) -> tensor<i8> {
+ ^bb0(%arg3: tensor<i8>, %arg4: tensor<i8>):
+ tosa.yield %arg3 : tensor<i8>
+ } else {
+ ^bb0(%arg3: tensor<i8>, %arg4: tensor<i8>):
+ tosa.yield %arg4 : tensor<i8>
+ }
+ return %0 : tensor<i8>
+}
|
|
@llvm/pr-subscribers-mlir-tosa Author: Luke Hutton (lhutton1) ChangesPrevioulsy the error_if check for controlflow operators would silently fail on valid controflow operators. This was due to incorrect return logic in the validation function. This commit fixes that logic. Full diff: https://github.com/llvm/llvm-project/pull/159754.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp b/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
index 790bbf77877bc..e6091df367754 100644
--- a/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
+++ b/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
@@ -1257,8 +1257,8 @@ bool checkErrorIfCondIf(Operation *op) {
// tosa.yield %arg4
// }
- return failed(checkIsolatedRegion(op, ifOp.getThenGraph(), "then")) ||
- failed(checkIsolatedRegion(op, ifOp.getElseGraph(), "else"));
+ return succeeded(checkIsolatedRegion(op, ifOp.getThenGraph(), "then")) &&
+ succeeded(checkIsolatedRegion(op, ifOp.getElseGraph(), "else"));
}
bool checkErrorIfWhileLoop(Operation *op) {
@@ -1266,8 +1266,8 @@ bool checkErrorIfWhileLoop(Operation *op) {
if (!whileOp)
return true;
- return failed(checkIsolatedRegion(op, whileOp.getCondGraph(), "cond")) ||
- failed(checkIsolatedRegion(op, whileOp.getBodyGraph(), "body"));
+ return succeeded(checkIsolatedRegion(op, whileOp.getCondGraph(), "cond")) &&
+ succeeded(checkIsolatedRegion(op, whileOp.getBodyGraph(), "body"));
}
bool checkErrorIfScatter(Operation *op) {
diff --git a/mlir/test/Dialect/Tosa/tosa-validation-valid-strict.mlir b/mlir/test/Dialect/Tosa/tosa-validation-valid-strict.mlir
new file mode 100644
index 0000000000000..fe423104359ab
--- /dev/null
+++ b/mlir/test/Dialect/Tosa/tosa-validation-valid-strict.mlir
@@ -0,0 +1,15 @@
+// RUN: mlir-opt %s -split-input-file -verify-diagnostics --tosa-validate="profile=pro_int,pro_fp extension=int16,int4,bf16,fp8e4m3,fp8e5m2,fft,variable,controlflow,doubleround,inexactround strict-op-spec-alignment" | FileCheck %s
+
+// -----
+
+// CHECK-LABEL: test_cond_if
+func.func @test_cond_if(%arg0: tensor<i8>, %arg1: tensor<i8>, %arg2: tensor<i1>) -> tensor<i8> {
+ %0 = tosa.cond_if %arg2 (%arg3 = %arg0, %arg4 = %arg1) : tensor<i1> (tensor<i8>, tensor<i8>) -> tensor<i8> {
+ ^bb0(%arg3: tensor<i8>, %arg4: tensor<i8>):
+ tosa.yield %arg3 : tensor<i8>
+ } else {
+ ^bb0(%arg3: tensor<i8>, %arg4: tensor<i8>):
+ tosa.yield %arg4 : tensor<i8>
+ }
+ return %0 : tensor<i8>
+}
|
Previoulsy the error_if check for controlflow operators would silently fail on valid controflow operators. This was due to incorrect return logic in the validation function. This commit fixes that logic. Change-Id: I3c69c726028ff387202eef1d1f56b3c40ce80157
lhutton1
force-pushed
the
fix-validation-controlflow
branch
from
cd5c325 to
1ae1cf8
Compare
|
Thanks, negative tests already exist here: https://github.com/llvm/llvm-project/blob/main/mlir/test/Dialect/Tosa/error_if_check.mlir#L231. I actually realised some positive tests also existed here, but they weren't being checked correctly since |
Gotcha, thanks. |
|
|
||
| // ----- | ||
|
|
||
| // CHECK-LABEL: test_cond_if_isolated_from_above |
There was a problem hiding this comment.
I'm not sure what is the value in those CHECK-LABEL since there are no CHECK between them. Since these tests try to check the absence of an error, the mlir-opt line should be enough and FileCheck is not needed.
There was a problem hiding this comment.
Likewise in error_if_check.mlir by the way which doesn't even run FileCheck. Can you do a separate patch to remove the CHECK directives in that file?
There was a problem hiding this comment.
Thanks, you're more familiar with FileCheck/LIT so appreciate any suggestion. Without this fix, these tests still passed without FileCheck. I suspect it's because they were returning a silent failure? So I used FileCheck here to help make sure that something was returned from mlir-opt and that the test would fail as expected without the fix.
I think you're correct that I didn't need to use CHECK-LABEL, I was just following the pattern used by most tests under TOSA.
There was a problem hiding this comment.
For checking the absence of an error you could just remove the --split-input-files and then mlir-opt should return an error if any of the test fail to compile. On the other hand having CHECK-LABEL means you'll get a more precise error of which test fail so maybe worth keeping. The CHECK-LABEL without a FileCheck invocation though is waiting for trouble because someone might add some tests thinking that FileCheck is being run and wrongly conclude that their test work.
Previoulsy the error_if check for controlflow operators would silently fail on valid controflow operators. This was due to incorrect return logic in the validation function. This commit fixes that logic.