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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

Thanks for the review @rriddle



================
Comment at: mlir/include/mlir/Dialect/Affine/Utils.h:24
+/// hoisting could lead to significant code expansion in some cases.
+bool hoistAffineIfOp(AffineIfOp ifOp, bool *folded = nullptr);
+
----------------
rriddle wrote:
> bondhugula wrote:
> > rriddle wrote:
> > > Why not LogicalResult?
> > It would always return success() then - this utility can't fail. (I didn't think 'no hoisting' should be called a failure, but if we do, we could use LogicalResult.)
> I would say that success/failure takes on different meanings depending on the context. Here I would read `success` as the if was successfully hoisted. This is different from the failure mode that was decided on for passes.  e.g., the folding methods returns success if the operation was folded, and failure otherwise.   
Sure - changed to LogicalResult then. 


================
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 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77870