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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked 5 inline comments as done.
bondhugula added a comment.

In D77870#1975018 <https://reviews.llvm.org/D77870#1975018>, @dcaballe wrote:

> Thanks for adding this optimization!
>  One general comment: this is called loop unswitching in LLVM: https://llvm.org/docs/Passes.html#loop-unswitch-unswitch-loops
>  It would be great if we could use that terminology for consistency.


Sure, thanks.



================
Comment at: mlir/include/mlir/Dialect/Affine/Utils.h:21
+/// Hoists out affine.if/else to as high as possible, i.e., past all invariant
+/// affine.fors/parallel's. Returns true if any hoisting happened or the `ifOp`
+/// was folded away; in the latter case `folded` is set to true. This
----------------
dcaballe wrote:
> Wondering what happens if the `if` is loop invariant to only some of the IVs in an affine.parallel. Do we plan to split the affine.parallel?
Good question. At the moment, we don't intend to split the affine.parallel. If needed, this could be made configurable.


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


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:66
+
+// Returns true if the hoisting happened or if the op was folded away.
+bool mlir::hoistAffineIfOp(AffineIfOp ifOp, bool *folded) {
----------------
rriddle wrote:
> Please use ///
This is an implementation comment to help understand the return value. The longer doc comment is on the declaration in the header - didn't want to duplicate it.


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:67
+// Returns true if the hoisting happened or if the op was folded away.
+bool mlir::hoistAffineIfOp(AffineIfOp ifOp, bool *folded) {
+  // Apply canonicalization patterns and folding - this is necessary for the
----------------
andydavis1 wrote:
> This function is well documented but a little long. Could it be broken up a bit? For example, the canonicalization bit a the top of the function could be pulled out into a separate function.
I think this is a good idea. But instead of the top part which is short, we could break the bottom half which has the mechanics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77870