[PATCH] D154925: [MLIR][IR] Rewrite OperationVerifier using worklist

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Thanks! I wasn't sure when I'll get bandwidth to get to it :)



================
Comment at: mlir/lib/IR/Verifier.cpp:58
 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);
+  using WorkItem = std::variant<Operation *, Block *>;
+
----------------
std::variant is gonna be 16B, LLVM has `PointerUnion` for this.

(see also the existing `class IRUnit` in `mlir/IR/Unit.h`)


================
Comment at: mlir/lib/IR/Verifier.cpp:82
   // Verify the operation first, collecting any IsolatedFromAbove operations.
   if (failed(verifyOperation(op)))
     return failure();
----------------
(This is the source of recursion I'm pointing below)


================
Comment at: mlir/lib/IR/Verifier.cpp:276
+      if (failed(std::visit(
+              [this](auto *workItem) { return verifyPostChildren(*workItem); },
+              back)))
----------------
Isn't verifyPostChildren gonna recurse on isolated regions?


================
Comment at: mlir/lib/IR/Verifier.cpp:283
+    // 1st visit of this work item ("entrance").
+    seen.insert(back);
+    if (failed(std::visit(
----------------
We should be able to avoid the double query to the hash map by doing:
```
bool secondVisit = !seen.insert(back).second;
if (secondVisit) {
  worklist.pop_back();
  ...
  continue;
}
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154925/new/

https://reviews.llvm.org/D154925