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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

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.



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


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


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:101
+
+  // Hoist the if/else right around `hoistOverOp`.
+  // Create the hoisted 'if' first. Then, clone the op we are hoisting over for
----------------
Comments refer to hoistOverOp but variable is hoistPoint? Please, align terminology.


================
Comment at: mlir/test/Dialect/Affine/if-hoist.mlir:12
+      affine.if affine_set<(d0) : (d0 - 2 >= 0)>(%i) {
+        affine.load %A[%j] : memref<100xi32>
+      }
----------------
Could we use different loads so that we can check that the guarded load is placed in the right place?


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


================
Comment at: mlir/test/Dialect/Affine/if-hoist.mlir:70
+      }
+    }
+  }
----------------
Could you please add another test that covers if-else + extra code in the innermost loop and check for the right order of the resulting instructions? Similar to the first test


================
Comment at: mlir/test/Dialect/Affine/if-hoist.mlir:183
+}
+
+// CHECK:       affine.for
----------------
could you please add some tests with parallel?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77870