[Mlir-commits] [mlir] [mlir][tosa] Check for isolated regions in `tosa.while_loop` (PR #144865)

[Jack Frankland]

================
@@ -1193,61 +1193,99 @@ 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);
+  });
+}
+
+static bool isRegionIsolatedFromAbove(Region &region) {
+  bool noLiveInValue = true;
+  region.walk([&noLiveInValue, &region](Operation *op) {
+    if (!isOpIsolatedFromAbove(op, region)) {
+      noLiveInValue = false;
+      return WalkResult::interrupt();
+    }
+    return WalkResult::advance();
+  });
+  return noLiveInValue;
 }
 
 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.
-
-  // Returns true if the region uses no external input operands.
-  auto isNullaryRegion = [](Region &region) -> bool {
-    bool noLiveInValue = true;
-    region.walk([&noLiveInValue](Operation *op) {
-      if (!isNullaryOperation(op)) {
-        noLiveInValue = false;
-        return WalkResult::interrupt();
-      }
-      return WalkResult::advance();
-    });
-    return noLiveInValue;
-  };
+  // 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.
+  Region &thenGraph = ifOp.getThenGraph();
+  Region &elseGraph = ifOp.getElseGraph();
+  bool isThenGraphIsolatedRegion = isRegionIsolatedFromAbove(thenGraph);
+  bool isElseGraphIsolatedRegion = isRegionIsolatedFromAbove(elseGraph);
+
+  if (!isThenGraphIsolatedRegion || !isElseGraphIsolatedRegion) {
+    op->emitOpError()
+        << "is not conformant to the TOSA specification. It requires the "
+           "then/else regions are isolated from above.\n";
+    return false;
+  }
+  return true;
+}
 
-  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 checkErrorIfWhileLoop(Operation *op) {
+  auto whileOp = dyn_cast<tosa::WhileOp>(op);
+  if (!whileOp)
+    return true;
 
-  if ((isInputListEmpty != isThenGraphNullaryRegion) ||
-      (isInputListEmpty != isElseGraphNullaryRegion)) {
+  Region &condGraph = whileOp.getCondGraph();
+  Region &bodyGraph = whileOp.getBodyGraph();
+  bool isCondGraphIsolatedRegion = isRegionIsolatedFromAbove(condGraph);
----------------
FranklandJack wrote:

`const`?

https://github.com/llvm/llvm-project/pull/144865