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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/lib/IR/Verifier.cpp:276
+      if (failed(std::visit(
+              [this](auto *workItem) { return verifyPostChildren(*workItem); },
+              back)))
----------------
alexander-shaposhnikov wrote:
> mehdi_amini wrote:
> > Isn't verifyPostChildren gonna recurse on isolated regions?
> correct, it does, however, i kinda thought it could be a rare case that this recursion would blow up (at least it doesn't on the aforementioned tests) + wanted to keep the parallel processing in place.
> Basically for the whole thing the hard part was getting the order as close to what we currently have as possible (otherwise we'd need to do a massive update of tests) /  and keeping the code ~simple at the same time,
> that's how i landed on the current approach, but I'm open to suggestions.
I don't think it's necessarily rare in general, it just happens that the kind of IR you're looking at seem to be mostly non-isolated ops I think.

So your patch isn't fixing it for all the case, but it seems like a major step in the right direction!
We can look into this last recursion later.


(Please call this out in the description)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154925