[PATCH] D77870: [MLIR] Introduce utility to hoist affine if/else conditions
River Riddle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 15 10:24:28 PDT 2020
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
More information about the llvm-commits
mailing list