From 25dc942e88ec0ddb8ad54505ff027d6b3be61f64 Mon Sep 17 00:00:00 2001 From: Luke Hutton Date: Wed, 11 Jun 2025 09:04:53 +0000 Subject: [PATCH] [mlir][tosa] Fix check for isolated regions in `tosa.cond_if` 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 --- .../Tosa/Transforms/TosaValidation.cpp | 68 ++++++++++++------- mlir/test/Dialect/Tosa/error_if_check.mlir | 40 +++++++++-- 2 files changed, 77 insertions(+), 31 deletions(-) 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(op) || isa(op) || - isa(op) || isa(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, %arg1: tensor, %arg2: tensor) -> tensor { - // 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, %arg1: tensor, %arg2: tensor) -> tensor { + // 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 + }, { + ^bb0(): + tosa.yield %arg1 : tensor + }) : (tensor) -> tensor + return %0 : tensor +} + +// ----- + +func.func @test_cond_if_simplified_form_not_isolated_from_above(%arg0: tensor, %arg1: tensor, %arg2: tensor) -> tensor { + // 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) { - %1 = tosa.add %arg0, %arg1 : (tensor, tensor) -> tensor - tosa.yield %1 : tensor + tosa.yield %arg0 : tensor } else { - %1 = tosa.sub %arg0, %arg1 : (tensor, tensor) -> tensor - tosa.yield %1 : tensor + tosa.yield %arg1 : tensor } return %0 : tensor } + +// ----- + +// COM: Check isolated cond_if's are valid +func.func @test_cond_if_isolated_from_above(%arg0: tensor, %arg1: tensor, %arg2: tensor) -> tensor { + %0 = "tosa.cond_if"(%arg2, %arg0, %arg1) ({ + ^bb0(%arg3: tensor, %arg4: tensor): + tosa.yield %arg3 : tensor + }, { + ^bb0(%arg3: tensor, %arg4: tensor): + tosa.yield %arg4 : tensor + }) : (tensor, tensor, tensor) -> tensor + return %0 : tensor +}