This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Extend fusion to support WAW atm on buffers.
ClosedPublic

Authored by hanchung on Mar 26 2020, 5:43 PM.

Details

Summary

The RAW fusion happens only if the produecer block dominates the consumer block.
The WAW pattern also works with the precondition. I.e., if a producer can
dominate the consumer, they can fairly fuse together.

Since they are all tilable, we can think the pattern like this way:

Input:

linalg_op1 view

tile_loop
  subview_2
  linalg_op2 subview_2

Tile the first Linalg op as same as the second Linalg.

tile_loop
  subview_1
  linalg_op1 subview_1

tile_loop
  subview_2
  liangl_op2 subview_2

Since the first Linalg op is tilable in the same way and the computation are
independently, it's fair to fuse it with the second Linalg op.

tile_loop
  subview_1
  linalg_op1 subview_1
  linalg_op2 subview_2

In short, this patch includes:

  • Handling both RAW and WAW pattern.
  • Adding a interface method to get input and output buffers.
  • Exposing a method to get a StringRef of a dependency type.
  • Fixing existing WAW tests and add one more use case: initialize the buffer before conv op.

Diff Detail

Event Timeline

hanchung created this revision.Mar 26 2020, 5:43 PM
hanchung updated this revision to Diff 253024.Mar 26 2020, 5:45 PM

Revert accidental change.

hanchung updated this revision to Diff 253025.Mar 26 2020, 6:01 PM

Fix formatting.

mravishankar accepted this revision.Mar 30 2020, 11:50 AM

LGTM. Please wait for @nicolasvasilache to accept.

mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td
105

Nit : rename to getInputAndOutputBuffers

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
335

Is WRW a typo?

338

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.

This revision is now accepted and ready to land.Mar 30 2020, 11:50 AM
nicolasvasilache accepted this revision.Mar 30 2020, 3:41 PM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td
105

This should be either getInputOrOutputBuffer or just getBuffer.
It returns one single buffer at position $i.

mlir/test/Dialect/Linalg/fusion.mlir
336

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.

hanchung updated this revision to Diff 254046.Mar 31 2020, 5:11 PM
hanchung marked 8 inline comments as done.

Address comments.

mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td
105

Yes, let's use getBuffer. It's simpler.

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
338

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.

mlir/test/Dialect/Linalg/fusion.mlir
336

Oh, I misread the result. Thank you to point it out. :)

This revision was automatically updated to reflect the committed changes.