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

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:141-142
+  ifOp.getCanonicalizationPatterns(patterns, ifOp.getContext());
+  bool erased;
+  applyOpPatternsAndFold(ifOp, patterns, &erased);
+  if (erased) {
----------------
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.


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:28
+static void promoteIfBlock(AffineIfOp ifOp, bool elseBlock) {
+  OpBuilder b(ifOp);
+
----------------
This builder looks unused.


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:96
+  // We'll set an attribute to identify this op in a clone of this sub-tree.
+  ifOp.setAttr(b.getIdentifier(idForIfOp), b.getBoolAttr(true));
+  hoistOverOpClone = b.clone(*hoistOverOp, operandMap);
----------------
b.getIdentifier should be unnecessary here, you already have an identifier.


================
Comment at: mlir/test/lib/Dialect/Affine/TestAffineLoopUnswitching.cpp:48
+    changed = false;
+    func.walk([&](AffineIfOp op) {
+      if (succeeded(hoistAffineIfOp(op))) {
----------------
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.


================
Comment at: mlir/test/lib/Dialect/Affine/TestAffineLoopUnswitching.cpp:63
+}
+
+} // namespace mlir
----------------
nit: drop the newline here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77870