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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Can you add test coverage?



================
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);
----------------
bondhugula wrote:
> 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.
> 
Yes, there is another one pending.


================
Comment at: mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp:253
+
+namespace {
+/// This is a simple driver for the PatternMatcher to apply patterns and perform
----------------
Can you use a comment block to split these?


================
Comment at: mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp:256
+/// folding on a single op. It repeatedly applies the locally optimal patterns
+/// in a roughly "bottom up" way.
+class OpPatternRewriteDriver : public PatternRewriter {
----------------
"bottom up" doesn't really make sense here.


================
Comment at: mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp:265
+
+  // This is no additional action to perform other than to just insert the op.
+  Operation *insert(Operation *op) override { return OpBuilder::insert(op); }
----------------
Please use /// for the top-level comments.


================
Comment at: mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp:329
+      return true;
+  } while (changed && ++i < maxIterations);
+
----------------
```
while (!erased && changed && ...)
```

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77487