[PATCH] D77870: [MLIR] Introduce utility to hoist affine if/else conditions

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked 17 inline comments as done.
bondhugula added inline comments.


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:60
+    }
+    // You can always hoist up past any affine.if ops.
+    res = parentOp;
----------------
dcaballe wrote:
> could you please add a couple of tests for the last two scenarios (line 56 and 60)?
Thanks - this wasn't covered earlier. 


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:159
+  applyPatternsAndFoldGreedily(
+      hoistedIfOp.getParentWithTrait<OpTrait::IsIsolatedFromAbove>(),
+      std::move(patterns));
----------------
rriddle wrote:
> I would have expected the if you want to remove dead else blocks you could specifically target the ops you know about, instead of running on the entire function.
I actually forgot to update this call to use the 'op local' apply patterns, which I used at the entry. But looking back, that won't yield the desired simplification because in some cases, we want some dead loops in the else version to go away (even if the 'else' isn't all dead). So, it's something between 'op local' or whole function level that we need here. Example: the following will yield a dead loop in the else branch after hoisting unless you run the rewriter on all ops inside the hoisted if (and on only those to be precise). We should support such a method in the rewrite driver.

```
for 
  x;
  for 
    if 
      y
```







================
Comment at: mlir/test/Dialect/Affine/if-hoist.mlir:57
+// CHECK-NEXT:       affine.for
+
+
----------------
dcaballe wrote:
> Maybe interesting to add a CHECK-NOT for else?
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77870