[PATCH] D76897: [mlir][Linalg] Extend fusion to support WAW atm on buffers.

[Han-Chung Wang via Phabricator via llvm-commits]

hanchung added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td:105
+      "Return the input or output buffer at the given index.",
+      "Value", "getInputsAndOutputsBuffer", (ins "unsigned":$i)
+    >,
----------------
nicolasvasilache wrote:
> mravishankar wrote:
> > Nit : rename to getInputAndOutputBuffers
> This should be either `getInputOrOutputBuffer` or just `getBuffer`.
> It returns one single buffer at position `$i`. 
Yes, let's use `getBuffer`. It's simpler.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:338
+    OpBuilder &b, LinalgOp consumer, unsigned consumerIdx,
+    const LinalgDependenceGraph &graph, OperationFolder *folder) {
+  SmallVector<LinalgDependenceGraph::DependenceType, 4> deps = {
----------------
mravishankar wrote:
> You could consider changing `fuseProducerOp` to take an extra argument `ArrayRef<LinalgDependenceGraph::DependenceType> deps` to allow clients to control which deps to fuse. You could have a default of RAW and WAW dependences.
It needs more change to expose LinalgDependenceGraph::DependenceType because it is a enum inside LinalgDependenceGraph.

In-class enum forward declaration is not possible, according to decl.enum.

btw, I don't think a case that user would like to control the deps to fuse? Let's make the change when there is a need.


================
Comment at: mlir/test/Dialect/Linalg/fusion.mlir:336
 // CHECK:  (%[[A:.*]]:{{.*}}, %[[B:.*]]:{{.*}}, %[[C:.*]]:{{.*}}, %[[D:.*]]:{{.*}}, %[[E:.*]]:{{.*}})
-// Cannot fuse C due to interleaved read of C that would be bypassed.
-// Cannot fuse E (WAW).
-// CHECK:  linalg.matmul
-// CHECK:  linalg.matmul
+// Fuse B and C, then fuse with A.
 // CHECK:  loop.for
----------------
nicolasvasilache wrote:
> This comment does not make sense to me.
> The original comment used `C` and `E` as a shortcut for `the producer of C` and `the producer of E`.
> 
> How about:
> `fuse the producer of E (WAW) then the producer of C (WAR)` ?
> 
> Also the resulting fusion is most likely a bug.
> It's not your fault and independent from this CL.
> I'll fix in a followup.
Oh, I misread the result. Thank you to point it out. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76897