[mlir][tosa] Fix check for isolated regions in tosa.cond_if
#143772
Merged
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
|
@llvm/pr-subscribers-mlir-tosa Author: Luke Hutton (lhutton1) ChangesThis commit fixes a check in the validation pass which intended to validate whether a Full diff: https://github.com/llvm/llvm-project/pull/143772.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp b/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
index d33fc902de3a1..067ee7d5a5c5a 100644
--- a/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
+++ b/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
@@ -1193,12 +1193,11 @@ bool checkErrorIfPad(Operation *op) {
return true;
}
-// Returns true if the operation takes no input operands, excluding attributes.
-static bool isNullaryOperation(Operation *op) {
- if (isa<tosa::ConstOp>(op) || isa<tosa::ConstShapeOp>(op) ||
- isa<tosa::YieldOp>(op) || isa<tosa::VariableOp>(op))
- return true;
- return false;
+static bool isOpIsolatedFromAbove(Operation *op, Region *region) {
+ return llvm::all_of(op->getOperands(), [&](auto operand) {
+ Region *operandRegion = operand.getParentRegion();
+ return region->isAncestor(operandRegion);
+ });
}
bool checkErrorIfCondIf(Operation *op) {
@@ -1206,19 +1205,43 @@ bool checkErrorIfCondIf(Operation *op) {
if (!ifOp)
return true;
- // Whether the types and shapes of operands between the input/output list and
- // internal regions are validated by the operation verifier. However, with
- // support for the simplified form - where redundant operand notations are
- // omitted - is not conformant to the specification. According to the
- // specification, all operands passed into an operation must be explicitly
- // declared at each operation's structure. This code section verify that the
- // operation's form complies with this requirement.
+ // Currently the dialect supports declaring cond_if operations that
+ // have then/else regions that reference values from outside these
+ // regions. According to the specification, all values used by the
+ // then/else regions must be explicitly declared within the regions.
+ // Therefore we must check that the then/else regions are
+ // "isolated from above", in order to be conformant to the
+ // specification.
+ //
+ // Note: the dialect currently supports two styles of syntax for
+ // declaring "cond_if" operations. We'll refer to these as follows:
+ //
+ // Generic:
+ // %0 = "tosa.cond_if"(%arg0, %arg1, %arg2) ({
+ // ^bb0(%arg3, %arg4):
+ // tosa.yield %arg3
+ // }, {
+ // ^bb0(%arg3, %arg4):
+ // tosa.yield %arg4
+ // })
+ //
+ // Simplified:
+ // %0 = tosa.cond_if %arg2 {
+ // tosa.yield %arg0
+ // } else {
+ // tosa.yield %arg1
+ // }
+ //
+ // Unfortunately, the simplified syntax does not encapsulate values
+ // used in then/else regions (see 'simplified' example above), so it
+ // must be rewritten to use the generic syntax in order to be conformant
+ // to the specification.
// Returns true if the region uses no external input operands.
- auto isNullaryRegion = [](Region ®ion) -> bool {
+ auto isIsolatedRegion = [](Region ®ion) -> bool {
bool noLiveInValue = true;
- region.walk([&noLiveInValue](Operation *op) {
- if (!isNullaryOperation(op)) {
+ region.walk([&noLiveInValue, ®ion](Operation *op) {
+ if (!isOpIsolatedFromAbove(op, ®ion)) {
noLiveInValue = false;
return WalkResult::interrupt();
}
@@ -1229,18 +1252,15 @@ bool checkErrorIfCondIf(Operation *op) {
mlir::Region &thenGraph = ifOp.getThenGraph();
mlir::Region &elseGraph = ifOp.getElseGraph();
- bool isThenGraphNullaryRegion = isNullaryRegion(thenGraph);
- bool isElseGraphNullaryRegion = isNullaryRegion(elseGraph);
- bool isInputListEmpty = ifOp.getInputList().size() == 0;
+ bool isThenGraphIsolatedRegion = isIsolatedRegion(thenGraph);
+ bool isElseGraphIsolatedRegion = isIsolatedRegion(elseGraph);
- if ((isInputListEmpty != isThenGraphNullaryRegion) ||
- (isInputListEmpty != isElseGraphNullaryRegion)) {
+ if (!isThenGraphIsolatedRegion || !isElseGraphIsolatedRegion) {
op->emitOpError()
- << "the current simplified form is not strictly conformant to the "
- "spec, please use the generic format\n";
+ << "is not conformant to the TOSA specification. It requires the "
+ "then/else regions are isolated from above.\n";
return false;
}
-
return true;
}
diff --git a/mlir/test/Dialect/Tosa/error_if_check.mlir b/mlir/test/Dialect/Tosa/error_if_check.mlir
index 1f25132d6bcf3..00c891d4afaa0 100644
--- a/mlir/test/Dialect/Tosa/error_if_check.mlir
+++ b/mlir/test/Dialect/Tosa/error_if_check.mlir
@@ -227,15 +227,41 @@ func.func @test_error_i32_unsigned_output(%arg0: tensor<1xi8>) -> tensor<1xi32>
}
// -----
-// CHECK-LABEL: cond_if_simplified_form
-func.func @test_cond_if_simplified_form(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
- // expected-error@+1 {{'tosa.cond_if' op the current simplified form is not strictly conformant to the spec, please use the generic format}}
+
+func.func @test_cond_if_not_isolated_from_above(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
+ // expected-error@+1 {{'tosa.cond_if' op is not conformant to the TOSA specification. It requires the then/else regions are isolated from above.}}
+ %0 = "tosa.cond_if"(%arg2) ({
+ ^bb0():
+ tosa.yield %arg0 : tensor<f32>
+ }, {
+ ^bb0():
+ tosa.yield %arg1 : tensor<f32>
+ }) : (tensor<i1>) -> tensor<f32>
+ return %0 : tensor<f32>
+}
+
+// -----
+
+func.func @test_cond_if_simplified_form_not_isolated_from_above(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
+ // expected-error@+1 {{'tosa.cond_if' op is not conformant to the TOSA specification. It requires the then/else regions are isolated from above.}}
%0 = tosa.cond_if %arg2 -> (tensor<f32>) {
- %1 = tosa.add %arg0, %arg1 : (tensor<f32>, tensor<f32>) -> tensor<f32>
- tosa.yield %1 : tensor<f32>
+ tosa.yield %arg0 : tensor<f32>
} else {
- %1 = tosa.sub %arg0, %arg1 : (tensor<f32>, tensor<f32>) -> tensor<f32>
- tosa.yield %1 : tensor<f32>
+ tosa.yield %arg1 : tensor<f32>
}
return %0 : tensor<f32>
}
+
+// -----
+
+// COM: Check isolated cond_if's are valid
+func.func @test_cond_if_isolated_from_above(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
+ %0 = "tosa.cond_if"(%arg2, %arg0, %arg1) ({
+ ^bb0(%arg3: tensor<f32>, %arg4: tensor<f32>):
+ tosa.yield %arg3 : tensor<f32>
+ }, {
+ ^bb0(%arg3: tensor<f32>, %arg4: tensor<f32>):
+ tosa.yield %arg4 : tensor<f32>
+ }) : (tensor<i1>, tensor<f32>, tensor<f32>) -> tensor<f32>
+ return %0 : tensor<f32>
+}
|
|
@llvm/pr-subscribers-mlir Author: Luke Hutton (lhutton1) ChangesThis commit fixes a check in the validation pass which intended to validate whether a Full diff: https://github.com/llvm/llvm-project/pull/143772.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp b/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
index d33fc902de3a1..067ee7d5a5c5a 100644
--- a/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
+++ b/mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
@@ -1193,12 +1193,11 @@ bool checkErrorIfPad(Operation *op) {
return true;
}
-// Returns true if the operation takes no input operands, excluding attributes.
-static bool isNullaryOperation(Operation *op) {
- if (isa<tosa::ConstOp>(op) || isa<tosa::ConstShapeOp>(op) ||
- isa<tosa::YieldOp>(op) || isa<tosa::VariableOp>(op))
- return true;
- return false;
+static bool isOpIsolatedFromAbove(Operation *op, Region *region) {
+ return llvm::all_of(op->getOperands(), [&](auto operand) {
+ Region *operandRegion = operand.getParentRegion();
+ return region->isAncestor(operandRegion);
+ });
}
bool checkErrorIfCondIf(Operation *op) {
@@ -1206,19 +1205,43 @@ bool checkErrorIfCondIf(Operation *op) {
if (!ifOp)
return true;
- // Whether the types and shapes of operands between the input/output list and
- // internal regions are validated by the operation verifier. However, with
- // support for the simplified form - where redundant operand notations are
- // omitted - is not conformant to the specification. According to the
- // specification, all operands passed into an operation must be explicitly
- // declared at each operation's structure. This code section verify that the
- // operation's form complies with this requirement.
+ // Currently the dialect supports declaring cond_if operations that
+ // have then/else regions that reference values from outside these
+ // regions. According to the specification, all values used by the
+ // then/else regions must be explicitly declared within the regions.
+ // Therefore we must check that the then/else regions are
+ // "isolated from above", in order to be conformant to the
+ // specification.
+ //
+ // Note: the dialect currently supports two styles of syntax for
+ // declaring "cond_if" operations. We'll refer to these as follows:
+ //
+ // Generic:
+ // %0 = "tosa.cond_if"(%arg0, %arg1, %arg2) ({
+ // ^bb0(%arg3, %arg4):
+ // tosa.yield %arg3
+ // }, {
+ // ^bb0(%arg3, %arg4):
+ // tosa.yield %arg4
+ // })
+ //
+ // Simplified:
+ // %0 = tosa.cond_if %arg2 {
+ // tosa.yield %arg0
+ // } else {
+ // tosa.yield %arg1
+ // }
+ //
+ // Unfortunately, the simplified syntax does not encapsulate values
+ // used in then/else regions (see 'simplified' example above), so it
+ // must be rewritten to use the generic syntax in order to be conformant
+ // to the specification.
// Returns true if the region uses no external input operands.
- auto isNullaryRegion = [](Region ®ion) -> bool {
+ auto isIsolatedRegion = [](Region ®ion) -> bool {
bool noLiveInValue = true;
- region.walk([&noLiveInValue](Operation *op) {
- if (!isNullaryOperation(op)) {
+ region.walk([&noLiveInValue, ®ion](Operation *op) {
+ if (!isOpIsolatedFromAbove(op, ®ion)) {
noLiveInValue = false;
return WalkResult::interrupt();
}
@@ -1229,18 +1252,15 @@ bool checkErrorIfCondIf(Operation *op) {
mlir::Region &thenGraph = ifOp.getThenGraph();
mlir::Region &elseGraph = ifOp.getElseGraph();
- bool isThenGraphNullaryRegion = isNullaryRegion(thenGraph);
- bool isElseGraphNullaryRegion = isNullaryRegion(elseGraph);
- bool isInputListEmpty = ifOp.getInputList().size() == 0;
+ bool isThenGraphIsolatedRegion = isIsolatedRegion(thenGraph);
+ bool isElseGraphIsolatedRegion = isIsolatedRegion(elseGraph);
- if ((isInputListEmpty != isThenGraphNullaryRegion) ||
- (isInputListEmpty != isElseGraphNullaryRegion)) {
+ if (!isThenGraphIsolatedRegion || !isElseGraphIsolatedRegion) {
op->emitOpError()
- << "the current simplified form is not strictly conformant to the "
- "spec, please use the generic format\n";
+ << "is not conformant to the TOSA specification. It requires the "
+ "then/else regions are isolated from above.\n";
return false;
}
-
return true;
}
diff --git a/mlir/test/Dialect/Tosa/error_if_check.mlir b/mlir/test/Dialect/Tosa/error_if_check.mlir
index 1f25132d6bcf3..00c891d4afaa0 100644
--- a/mlir/test/Dialect/Tosa/error_if_check.mlir
+++ b/mlir/test/Dialect/Tosa/error_if_check.mlir
@@ -227,15 +227,41 @@ func.func @test_error_i32_unsigned_output(%arg0: tensor<1xi8>) -> tensor<1xi32>
}
// -----
-// CHECK-LABEL: cond_if_simplified_form
-func.func @test_cond_if_simplified_form(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
- // expected-error@+1 {{'tosa.cond_if' op the current simplified form is not strictly conformant to the spec, please use the generic format}}
+
+func.func @test_cond_if_not_isolated_from_above(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
+ // expected-error@+1 {{'tosa.cond_if' op is not conformant to the TOSA specification. It requires the then/else regions are isolated from above.}}
+ %0 = "tosa.cond_if"(%arg2) ({
+ ^bb0():
+ tosa.yield %arg0 : tensor<f32>
+ }, {
+ ^bb0():
+ tosa.yield %arg1 : tensor<f32>
+ }) : (tensor<i1>) -> tensor<f32>
+ return %0 : tensor<f32>
+}
+
+// -----
+
+func.func @test_cond_if_simplified_form_not_isolated_from_above(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
+ // expected-error@+1 {{'tosa.cond_if' op is not conformant to the TOSA specification. It requires the then/else regions are isolated from above.}}
%0 = tosa.cond_if %arg2 -> (tensor<f32>) {
- %1 = tosa.add %arg0, %arg1 : (tensor<f32>, tensor<f32>) -> tensor<f32>
- tosa.yield %1 : tensor<f32>
+ tosa.yield %arg0 : tensor<f32>
} else {
- %1 = tosa.sub %arg0, %arg1 : (tensor<f32>, tensor<f32>) -> tensor<f32>
- tosa.yield %1 : tensor<f32>
+ tosa.yield %arg1 : tensor<f32>
}
return %0 : tensor<f32>
}
+
+// -----
+
+// COM: Check isolated cond_if's are valid
+func.func @test_cond_if_isolated_from_above(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
+ %0 = "tosa.cond_if"(%arg2, %arg0, %arg1) ({
+ ^bb0(%arg3: tensor<f32>, %arg4: tensor<f32>):
+ tosa.yield %arg3 : tensor<f32>
+ }, {
+ ^bb0(%arg3: tensor<f32>, %arg4: tensor<f32>):
+ tosa.yield %arg4 : tensor<f32>
+ }) : (tensor<i1>, tensor<f32>, tensor<f32>) -> tensor<f32>
+ return %0 : tensor<f32>
+}
|
FranklandJack
requested changes
Jul 16, 2025
udaya-ranga
reviewed
Jul 16, 2025
udaya-ranga
reviewed
Jul 16, 2025
udaya-ranga
reviewed
Jul 16, 2025
This commit fixes a check in the validation pass which intended to validate whether a `tosa.cond_if` operation was conformant to the specification. The specification requires all values used in the then/else regions are explicitly declared within the regions. This change checks that these regions are 'isolated from above', to ensure this requirement is true. Change-Id: I1b6eac1ed571e6b1eda4a58f0677c80e22977e58
lhutton1
force-pushed
the
fix-cond-if-isolated
branch
from
25dc942 to
fd0487e
Compare
lhutton1
commented
Jul 18, 2025
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
- check then/else region isolation independently - renamed isOpIsolatedFromAbove to isOpIsolatedWithinRegion - remove "COM" prefix in lit test Change-Id: I5988ba2e75c9aa81c57321628d49d35634847f03
lhutton1
force-pushed
the
fix-cond-if-isolated
branch
from
fd0487e to
869355b
Compare
|
LGTM! |
FranklandJack
approved these changes
Jul 21, 2025
This was referenced Jul 23, 2025
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
…43772) This commit fixes a check in the validation pass which intended to validate whether a `tosa.cond_if` operation was conformant to the specification. The specification requires all values used in the then/else regions are explicitly declared within the regions. This change checks that these regions are 'isolated from above', to ensure this requirement is true.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This commit fixes a check in the validation pass which intended to validate whether a
tosa.cond_ifoperation was conformant to the specification. The specification requires all values used in the then/else regions are explicitly declared within the regions. This change checks that these regions are 'isolated from above', to ensure this requirement is true.