[PATCH] D77870: [MLIR] Introduce utility for affine loop unswitching / hoisting if/else

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

Thanks for the review.



================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:141-142
+  ifOp.getCanonicalizationPatterns(patterns, ifOp.getContext());
+  bool erased;
+  applyOpPatternsAndFold(ifOp, patterns, &erased);
+  if (erased) {
----------------
rriddle wrote:
> bondhugula wrote:
> > @rriddle Here is where we need the `erased` parameter. I can't think of an alternative. You need to know whether the op was erased if this canonicalization is called. The user of this utility may not call the canonicalization nor could we expect them. Even if they do, they'd themselves have to potentially deal with the aftermath of the op getting erased. 
> Sorry, I don't think I was clear in the message in the other revision. I think we should remove the bool "converged" result that the methods are currently returning. I agree that knowing if the ops was erased or not can be important.
I see; sure, but it appears as if it would definitely be needed some day. 


================
Comment at: mlir/test/lib/Dialect/Affine/TestAffineLoopUnswitching.cpp:48
+    changed = false;
+    func.walk([&](AffineIfOp op) {
+      if (succeeded(hoistAffineIfOp(op))) {
----------------
rriddle wrote:
> nit: You could write this as:
> 
> ```
> auto walkFn = ([&](AffineIfOp op) -> WalkResult {
>   return hoistAffineIfOp(op);
> };
> if (func.walk(walkFn).wasInterrupted())
>   break;
> ```
> 
> You may even be able to use `hoistAffineIfOp` directly as walkFn, but I can't remember.
Thanks - I didn't know about wasInterrupted(). Note that the walk continues here if the  hoist fails, and is interrupted when the hoist succeeds. So I can't implicitly convert as you show above, but have used the pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77870