[PATCH] D74544: [MLIR] Add naive fusion of parallel loops.

[Stephan Herhut via Phabricator via llvm-commits]

herhut requested changes to this revision.
herhut added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopFusion.cpp:55
+  firstPloop.getBody()->walk([&](StoreOp store) {
+    bufferStores[store.getMemRef()].push_back(store.indices());
+  });
----------------
Storing to a locally defined buffer also should bail.


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopFusion.cpp:90
+/// in secondPloop.
+static bool haveNoReadWritesToSameBufs(ParallelOp firstPloop,
+                                       ParallelOp secondPloop) {
----------------
This should be the dual to the above function with the same comments regarding aliasing. Without alias information, we have to bail on the case where we load or store to a locally defined buffer. 


================
Comment at: mlir/test/Dialect/Loops/parallel-loop-fusion.mlir:32
+  %c1 = constant 1 : index
+  %sum = alloc() {temp = true} : memref<2x2xf32>
+  loop.parallel (%i, %j) = (%c0, %c0) to (%c2, %c2) step (%c1, %c1) {
----------------
Can you drop the `{temp = true}` from the tests. They are just noise here.


================
Comment at: mlir/test/Dialect/Loops/parallel-loop-fusion.mlir:276
+    %k = addi %i, %c1 : index
+    %sum_elem = load %sum[%k, %j] : memref<2x2xf32>
+    %A_elem = load %A[%i, %j] : memref<2x2xf32>
----------------
This would already fail due to not-matching indices. Maybe load from `A` instead?


================
Comment at: mlir/test/Dialect/Loops/parallel-loop-fusion.mlir:279
+    %product_elem = mulf %sum_elem, %A_elem : f32
+    store %product_elem, %sum[%i, %j] : memref<2x2xf32>
+    "loop.terminator"() : () -> ()
----------------
Why is this case illegal? If they store to the same buf but at the same index it should be fine.


================
Comment at: mlir/test/Dialect/Loops/parallel-loop-fusion.mlir:291
+
+func @do_not_fuse_loops_with_memref_defined_in_loop_bodies() {
+  %c2 = constant 2 : index
----------------
There are 4 cases for this. Read/write to local allocation in loop1/loop2. Essentially any effect on a memref we do not understand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74544