Skip to content

[mlir][tosa] Fix check for isolated regions in tosa.cond_if #143772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+77 −31
Open

[mlir][tosa] Fix check for isolated regions in tosa.cond_if #143772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 44 additions & 24 deletions mlir/lib/Dialect/Tosa/Transforms/TosaValidation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1193,32 +1193,55 @@ 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the relation of region to op ?
Is it the aboveRegion ? If so, can we clarify the concept in name please ?

return llvm::all_of(op->getOperands(), [&](auto operand) {
Region *operandRegion = operand.getParentRegion();
return region->isAncestor(operandRegion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "ancestor" relationship always equate to "IsolatedFromAbove" ?
If I compare an Op with a non-ancestor and non-overlapping region (Maybe a peer to ancestor), then also it is true that the Op is "IsolatedFromAbove" ?

});
}

bool checkErrorIfCondIf(Operation *op) {
auto ifOp = dyn_cast<tosa::IfOp>(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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both blocks here use the label bb0, is that expected? This is also the case for the lit tests below.

// 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 &region) -> bool {
auto isIsolatedRegion = [](Region &region) -> bool {
bool noLiveInValue = true;
region.walk([&noLiveInValue](Operation *op) {
if (!isNullaryOperation(op)) {
region.walk([&noLiveInValue, &region](Operation *op) {
if (!isOpIsolatedFromAbove(op, &region)) {
noLiveInValue = false;
return WalkResult::interrupt();
}
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst we are here, could we make these bools const?

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 "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weak preference for testing each condition seperately so we can add more info to the error message e.g. requires the "then" region be isolated from above.

"then/else regions are isolated from above.\n";
return false;
}

return true;
}

Expand Down
40 changes: 33 additions & 7 deletions mlir/test/Dialect/Tosa/error_if_check.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this test (and the one below) have values in both the then and else blocks that are not isolated from above? Should we add a second test and check each block seperately?

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to use the COM directive? Unless I'm mistaken there aren't any strings in this comment that would match another FileCheck directive.

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>):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is relation of arg3 to (%arg2, %arg0, %arg1) ?

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>
}
Loading