[mlir] Fix verification order of nested ops.

[Chia-hungDuan]

Summary

In order to increase parallism, certain ops with regions and have the
IsIsolatedFromAbove trait will have their verification delayed. That
means the region verifier may access the invalid ops and may lead to a
crash.

[rriddle]

Is there a test we can add that highlights the bug?

[Chia-hungDuan]

Add test cases

[Chia-hungDuan]

@rriddle, a gentle ping, what do you think about the tests?

[rriddle]

mlir/test/lib/Dialect/Test/TestDialect.cpp

1310

Why do we need this remark? (same below)

1321

Can we do mlir::verify on this just to be explicit?

[Chia-hungDuan]

mlir/test/lib/Dialect/Test/TestDialect.cpp

1310

These are used for indicating the verify()/verifyRegion() has executed and ensure the invocation order of all verifiers is sustained.

For example, one of the test cases is, we expect seeing a verification failure in nested op and we also expect that the verify() should be executed. Then we don't miss the case like, we've verified non region related traits but forget to call Op::verify(). The same as verifyRegion().

[Chia-hungDuan]

Address review comments

[Chia-hungDuan]

Rebase

mlir/lib/IR/Verifier.cpp

public:
   /// nested operations.
   explicit OperationVerifier(bool verifyRecursively)
       : verifyRecursively(verifyRecursively) {}
 
   /// Verify the given operation.
   LogicalResult verifyOpAndDominance(Operation &op);
 
 private:
+  /// Any ops that have regions and are marked as "isolated from above" will be
+  /// returned in the opsWithIsolatedRegions vector.
   LogicalResult
   verifyBlock(Block &block,
               SmallVectorImpl<Operation *> &opsWithIsolatedRegions);
-  /// Verify the properties and dominance relationships of this operation,
-  /// stopping region recursion at any "isolated from above operations".  Any
-  /// such ops are returned in the opsWithIsolatedRegions vector.
-  LogicalResult
-  verifyOperation(Operation &op,
-                  SmallVectorImpl<Operation *> &opsWithIsolatedRegions);
+
+  /// Verify the properties and dominance relationships of this operation.
+  LogicalResult verifyOperation(Operation &op);
 
   /// Verify the dominance property of regions contained within the given
   /// Operation.
   LogicalResult verifyDominanceOfContainedRegions(Operation &op,
                                                   DominanceInfo &domInfo);
 
   /// A flag indicating if this verifier should recursively verify nested
   /// operations.
   bool verifyRecursively;
 };
 } // namespace
 
 LogicalResult OperationVerifier::verifyOpAndDominance(Operation &op) {
-  SmallVector<Operation *> opsWithIsolatedRegions;
-
   // Verify the operation first, collecting any IsolatedFromAbove operations.
-  if (failed(verifyOperation(op, opsWithIsolatedRegions)))
+  if (failed(verifyOperation(op)))
     return failure();
 
   // Since everything looks structurally ok to this point, we do a dominance
   // check for any nested regions. We do this as a second pass since malformed
   // CFG's can cause dominator analysis construction to crash and we want the
   // verifier to be resilient to malformed code.
   if (op.getNumRegions() != 0) {
     DominanceInfo domInfo;
     if (failed(verifyDominanceOfContainedRegions(op, domInfo)))
       return failure();
   }
 
-  // If we aren't verifying nested operations, then we're done.
-  if (!verifyRecursively)
-    return success();
-
-  // Otherwise, check the dominance properties and invariants of any operations
-  // in the regions contained by the 'opsWithIsolatedRegions' operations.
-  return failableParallelForEach(
-      op.getContext(), opsWithIsolatedRegions,
-      [&](Operation *op) { return verifyOpAndDominance(*op); });
 }
 
 /// Returns true if this block may be valid without terminator. That is if:
 /// - it does not have a parent region.
 /// - Or the parent region have a single block and:
 ///    - This region does not have a parent op.
 ///    - Or the parent op is unregistered.
 ///    - Or the parent op has the NoTerminator trait.
for (Operation &op : block) {
 
     // If this operation has regions and is IsolatedFromAbove, we defer
     // checking.  This allows us to parallelize verification better.
     if (op.getNumRegions() != 0 &&
         op.hasTrait<OpTrait::IsIsolatedFromAbove>()) {
       opsWithIsolatedRegions.push_back(&op);
 
       // Otherwise, check the operation inline.
-    } else if (failed(verifyOperation(op, opsWithIsolatedRegions))) {
+    } else if (failed(verifyOperation(op))) {
       return failure();
     }
   }
 
   // Verify that this block is not branching to a block of a different
   // region.
   for (Block *successor : block.getSuccessors())
     if (successor->getParent() != block.getParent())
return block.back().emitError("block with no terminator, has ")
            << terminator;
 
   return success();
 }
 
 /// Verify the properties and dominance relationships of this operation,
 /// stopping region recursion at any "isolated from above operations".  Any such
 /// ops are returned in the opsWithIsolatedRegions vector.
-LogicalResult OperationVerifier::verifyOperation(
-    Operation &op, SmallVectorImpl<Operation *> &opsWithIsolatedRegions) {
+LogicalResult OperationVerifier::verifyOperation(Operation &op) {
   // Check that operands are non-nil and structurally ok.
   for (auto operand : op.getOperands())
     if (!operand)
       return op.emitError("null operand found");
 
   /// Verify that all of the attributes are okay.
   for (auto attr : op.getAttrs()) {
     // Check for any optional dialect specific attributes.
     if (auto *dialect = attr.getNameDialect())
       if (failed(dialect->verifyOperationAttribute(&op, attr)))
         return failure();
   }
 
   // If we can get operation info for this, check the custom hook.
   OperationName opName = op.getName();
   Optional<RegisteredOperationName> registeredInfo = opName.getRegisteredInfo();
   if (registeredInfo && failed(registeredInfo->verifyInvariants(&op)))
     return failure();
 
+  SmallVector<Operation *> opsWithIsolatedRegions;
+
   if (unsigned numRegions = op.getNumRegions()) {
     auto kindInterface = dyn_cast<RegionKindInterface>(op);
 
     // Verify that all child regions are ok.
     MutableArrayRef<Region> regions = op.getRegions();
     for (unsigned i = 0; i < numRegions; ++i) {
       Region &region = regions[i];
       RegionKind kind =
for (unsigned i = 0; i < numRegions; ++i) {
       if (verifyRecursively) {
         for (Block &block : region)
           if (failed(verifyBlock(block, opsWithIsolatedRegions)))
             return failure();
       }
     }
   }
 
+  // Verify the nested ops that are able to be verified in parallel.
+  if (failed(failableParallelForEach(
+          op.getContext(), opsWithIsolatedRegions,
+          [&](Operation *op) { return verifyOpAndDominance(*op); })))
+    return failure();
+
   // After the region ops are verified, run the verifiers that have additional
   // region invariants need to veirfy.
   if (registeredInfo && failed(registeredInfo->verifyRegionInvariants(&op)))
     return failure();
 
   // If this is a registered operation, there is nothing left to do.
   if (registeredInfo)
     return success();

mlir/test/Dialect/Func/invalid.mlir

 // RUN: mlir-opt -split-input-file %s -verify-diagnostics
 
 func @unsupported_attribute() {
   // expected-error @+1 {{invalid kind of attribute specified}}
   %0 = constant "" : index
   return
 }
 
 // -----
 
-func @return_i32_f32() -> (i32, f32)
+func private @return_i32_f32() -> (i32, f32)
 
 func @call() {
   // expected-error @+3 {{op result type mismatch at index 0}}
   // expected-note @+2 {{op result types: 'f32', 'i32'}}
   // expected-note @+1 {{function result types: 'i32', 'f32'}}
   %0:2 = call @return_i32_f32() : () -> (f32, i32)
   return
 }

mlir/test/Dialect/MemRef/invalid.mlir

 func @nonexistent_global_memref() {
   // expected-error @+1 {{'gv' does not reference a valid global memref}}
   %0 = memref.get_global @gv : memref<3xf32>
   return
 }
 
 // -----
 
-func @foo()
+func private @foo()
 
 func @nonexistent_global_memref() {
   // expected-error @+1 {{'foo' does not reference a valid global memref}}
   %0 = memref.get_global @foo : memref<3xf32>
   return
 }
 
 // -----

mlir/test/IR/invalid.mlir

 
 // -----
 
 // expected-error@+1 {{expected non-function type}}
 func @func_variadic(...)
 
 // -----
 
-func @redef()  // expected-note {{see existing symbol definition here}}
-func @redef()  // expected-error {{redefinition of symbol named 'redef'}}
+func private @redef()  // expected-note {{see existing symbol definition here}}
+func private @redef()  // expected-error {{redefinition of symbol named 'redef'}}
 
 // -----
 
 func @foo() {
 ^bb0:
   %x = constant @foo : (i32) -> ()  // expected-error {{reference to function with mismatched type}}
   return
 }

mlir/test/IR/test-verification-order.mlir

+// RUN: mlir-opt --mlir-disable-threading -split-input-file -verify-diagnostics %s
+
+func @verify_operand_type() {
+  %0 = arith.constant 1 : index
+  // expected-error@+1 {{op operand #0 must be 32-bit signless integer, but got 'index'}}
+  "test.verifiers"(%0) ({
+    %1 = arith.constant 2 : index
+  }) : (index) -> ()
+  return
+}
+
+// -----
+
+func @verify_nested_op_block_trait() {
+  %0 = arith.constant 1 : i32
+  // expected-remark@+1 {{success run of verifier}}
+  "test.verifiers"(%0) ({
+    %1 = arith.constant 2 : index
+    // expected-error@+1 {{op requires one region}}
+    "test.verifiers"(%1) : (index) -> ()
+  }) : (i32) -> ()
+  return
+}
+
+// -----
+
+func @verify_nested_op_operand() {
+  %0 = arith.constant 1 : i32
+  // expected-remark@+1 {{success run of verifier}}
+  "test.verifiers"(%0) ({
+    %1 = arith.constant 2 : index
+    // expected-error@+1 {{op operand #0 must be 32-bit signless integer, but got 'index'}}
+    "test.verifiers"(%1) ({
+      %2 = arith.constant 3 : index
+    }) : (index) -> ()
+  }) : (i32) -> ()
+  return
+}
+
+// -----
+
+func @verify_nested_isolated_above() {
+  %0 = arith.constant 1 : i32
+  // expected-remark@+1 {{success run of verifier}}
+  "test.verifiers"(%0) ({
+    // expected-remark@-1 {{success run of region verifier}}
+    %1 = arith.constant 2 : i32
+    // expected-remark@+1 {{success run of verifier}}
+    "test.verifiers"(%1) ({
+      // expected-remark@-1 {{success run of region verifier}}
+      %2 = arith.constant 3 : index
+    }) : (i32) -> ()
+  }) : (i32) -> ()
+  return
+}

mlir/test/lib/Dialect/Test/TestDialect.cpp

 #include "mlir/Dialect/Arithmetic/IR/Arithmetic.h"
 #include "mlir/Dialect/DLTI/DLTI.h"
 #include "mlir/Dialect/Func/IR/FuncOps.h"
 #include "mlir/Dialect/Tensor/IR/Tensor.h"
 #include "mlir/IR/BuiltinOps.h"
 #include "mlir/IR/DialectImplementation.h"
 #include "mlir/IR/PatternMatch.h"
 #include "mlir/IR/TypeUtilities.h"
+#include "mlir/IR/Verifier.h"
 #include "mlir/Reducer/ReductionPatternInterface.h"
 #include "mlir/Transforms/FoldUtils.h"
 #include "mlir/Transforms/InliningUtils.h"
 #include "llvm/ADT/StringSwitch.h"
 
 // Include this before the using namespace lines below to
 // test that we don't have namespace dependencies.
 #include "TestOpsDialect.cpp.inc"

 void SingleNoTerminatorCustomAsmOp::print(OpAsmPrinter &printer) {
   printer.printRegion(
       getRegion(), /*printEntryBlockArgs=*/false,
       // This op has a single block without terminators. But explicitly mark
       // as not printing block terminators for testing.
       /*printBlockTerminators=*/false);
 }
 
+//===----------------------------------------------------------------------===//
+// TestVerifiersOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult TestVerifiersOp::verify() {
+  if (!getRegion().hasOneBlock())
+    return emitOpError("`hasOneBlock` trait hasn't been verified");
+
+  Operation *definingOp = getInput().getDefiningOp();
+  if (definingOp && failed(::verify(definingOp)))
+    return emitOpError("operand hasn't been verified");
+
+  emitRemark("success run of verifier");
+
+  return success();
+}
+
+LogicalResult TestVerifiersOp::verifyRegions() {
+  if (!getRegion().hasOneBlock())
+    return emitOpError("`hasOneBlock` trait hasn't been verified");
+
+  for (Block &block : getRegion())
+    for (Operation &op : block)
+      if (failed(::verify(&op)))
+        return emitOpError("nested op hasn't been verified");
+
+  emitRemark("success run of region verifier");
+
+  return success();
+}
+
 #include "TestOpEnums.cpp.inc"
 #include "TestOpInterfaces.cpp.inc"
 #include "TestOpStructs.cpp.inc"
 #include "TestTypeInterfaces.cpp.inc"
 
 #define GET_OP_CLASSES
 #include "TestOps.cpp.inc"

mlir/test/lib/Dialect/Test/TestOps.td

let arguments = (ins
         );
 
     let results = (outs Res<AnyMemRef, "", [MemAlloc<TestResource>]>);
 }
 
 def TestEffectsOpB : TEST_Op<"op_with_effects_b",
     [MemoryEffects<[MemWrite<TestResource>]>]>;
 
+//===----------------------------------------------------------------------===//
+// Test Ops with verifiers
+//===----------------------------------------------------------------------===//
+
+def TestVerifiersOp : TEST_Op<"verifiers",
+                              [SingleBlock, NoTerminator, IsolatedFromAbove]> {
+  let arguments = (ins I32:$input);
+  let regions = (region SizedRegion<1>:$region);
+  let hasVerifier = 1;
+  let hasRegionVerifier = 1;
+}
+
 #endif // TEST_OPS