[PATCH] D73190: [mlir] Adds affine loop fusion transformation function to LoopFusionUtils.

[Diego Caballero via Phabricator via llvm-commits]

dcaballe requested changes to this revision.
dcaballe added a comment.
This revision now requires changes to proceed.

Thanks for working on this Andy! Some minor comments



================
Comment at: mlir/test/Transforms/loop-fusion-transformation.mlir:3
+
+// -----
+
----------------
nit: remove this first `-----` so that we don't process an empty chunk?


================
Comment at: mlir/test/Transforms/loop-fusion-transformation.mlir:17
+  // CHECK-NEXT:   affine.store %{{.*}}, %{{.*}}[%[[IV0]]] : memref<100xf32>
+  // CHECK-NEXT:   %{{.*}} = affine.load %{{.*}}[%[[IV0]]] : memref<100xf32>
+  // CHECK-NEXT: }
----------------
nit: for all these tests, please remove unnecessary lhs (e.g., `%{{.*}} =`)  for loads, addf, and other ops if we don't capture their output in a variable


================
Comment at: mlir/test/lib/Transforms/TestLoopFusion.cpp:187
+  DenseMap<unsigned, SmallVector<AffineForOp, 2>> depthToLoops;
+  if (clTestLoopFusionTransformation) {
+    // Run loop fusion until a fixed point is reached.
----------------
Would it make sense to have only the fixed point optimization "mode"? Do we need to keep the old mode for some reason (other than, maybe, fixing some tests)?
If so, probably better to create an independent pass (`TestFixedPointLoopFusion`?) for the fixed point mode since it's independent from the other mode. In that way, we wouldn't need to use both flags `-test-loop-fusion -test-loop-fusion-transformation` for testing. 



================
Comment at: mlir/test/lib/Transforms/TestLoopFusion.cpp:196
       }
+      // Trye to fuse all combinations of src/dst loop nests in 'depthToLoops'.
+      changed = iterateLoops(depthToLoops, testLoopFusionTransformation);
----------------
Trye -> Try


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73190