[PATCH] D77487: [MLIR] [WIP] Introduce applyOpPatternsAndFold for op local rewrites

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked an inline comment as done.
bondhugula added inline comments.


================
Comment at: mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp:251
+    // notified of any necessary changes, so there is nothing else to do here.
+    changed |= matcher.matchAndRewrite(op, *this);
+  } while (changed && ++i < maxIterations);
----------------
rriddle wrote:
> bondhugula wrote:
> > rriddle wrote:
> > > This is wrong, you don't handle when the op is erased by a pattern. This really feels like should be its own driver, it doesn't really share anything.
> > Hmm... if the op was erased in a pattern by just erase(), there is no way to know if it was erased? If the op is erased via eraseOp or replaceOp, checking for null at the op's position via worklistMap will address that (we'll have to just keep the op in the worklist before matchAndRewrite). The other issue is that the erased op's operands will also get into the worklist and we don't want them to, but this can be addressed by clearing out the worklist. This could use a separate driver as you say, but it doesn't immediately address the first issue - how do we detect erasure if done via op.erase()?
> > how do we detect erasure if done via op.erase()?
> We don't have to. It isn't valid to do IR mutations outside of the PatternRewriter.
> 
> I thought this was documented somewhere, but https://mlir.llvm.org/docs/QuickstartRewrites/ seems to be extremely out of date. I have an in-progress update to https://mlir.llvm.org/docs/Canonicalization/ I'll add that in as part of it.
I don't recall that requirement ever being documented, although at one point some instances of op.erase() were replaced by rewriter.eraseOp(op). The document itself hasn't changed core content wise since Mar 2019. I couldn't find it in your recent update to the canonicalization doc. Is another one pending?

If that's the constraint on how one can erase, then I think I have it implemented correctly now. It needs better testing though.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77487