This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Mostly NFC: Change fusion on tensor to use RewritePatterns.
ClosedPublic

Authored by mravishankar on Oct 11 2020, 1:41 AM.

Details

Reviewers
nicolasvasilache
Summary

Some of the fusions used in the LinalgFusionOfTensorOpsPass can be
treated as regular folding patterns. To allow for this, refactor the
fusion logic to execute within different patterns. These patterns can
be later moved into folding/canonicalize patterns, but for now just
provide a hook to add these patterns to the pattern list.

Depends on D89002

Diff Detail

Event Timeline

mravishankar created this revision.Oct 11 2020, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2020, 1:41 AM
mravishankar requested review of this revision.Oct 11 2020, 1:41 AM
nicolasvasilache accepted this revision.Oct 14 2020, 7:58 AM

Looks good overall.
Noted a bunch of places where it should be possible to significantly reduce code size / legibility with some mechanical refactorings.

Thanks @mravishankar !

mlir/include/mlir/Dialect/Linalg/Passes.h
59

s/an/a

mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
595

could you please put some pseudo-IR snippet to doc this?

605

Can we refactor this loop into a condition using llvm::all_of that captures the index?
Then we can just exit early here and remove most of the code from under the loop.

It was confusing to me to see that we have a loop but we always at most execute only 1 iteration.

609

getInputIndexingMap would not assume that you know the internal storage order of maps.

620

llvm::to_vector<4>(op.indexing_maps().getAsValueRange<AffineMapAttr>());
should work?

642

getAsValueRange

646

Can we update in place and simplify the logic around this or do you expect it to be more complex?
Maybe that could let us drop createLinalgOpOfSameType too.
This seems to be more generally applicable and maybe I can also drop clone and create from the LinalgStructuredOpInterface.

660

This seems like something you should let DCE do?

677

Same comment about restructuring with llvm::all_of that captures the first element you want and dropping the nesting under this "execute at most once"-loop.

697

same comment re DCE.

723

getAsValueRange

750

Same Q re in-place update.

763

DCE?

808

Same remark re loop/getAsValueRange/DCE as above.
Same Q on DCE as above.

895

Please refactor at most one loop.

905

DCE.

This revision is now accepted and ready to land.Oct 14 2020, 7:58 AM
mravishankar marked 7 inline comments as done.

Addressing comments.

I addressed some of the comments. Others I will address in a follow up CL to not make these stack of changes already larger. Thanks for the review!

mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
605

This part is just a refactor of the logic that existed earlier. I can fix this up in a later CL. These set of changes are already quite big.

620

Thanks!

642

The base container is not an ArrayAttr. But I can use rewriter.getAffineMapArrayAttr at the create method. So I can remove these lines.

646

Good idea. I will try it out.

660

Is there a DCE pass? Would be painful to write tests without it. For now leaving it as is. Ill do another follow up with addressing these comments.

677

Sure thing. WIll address this in a follow up (though its simpler here).

697

Please see above.

750

Here the linalg.generic/indexed_generic is the producer. Not sure its a good idea to replace the operation in place. Its better to create a new fused operation cause the uses of the consumer linalg.tensor_reshape is being replaced with the new op.

808

The loop question, ill do in a follow up. Used getAsValueRange. Maybe inplace update is possible, but I need to signature convert the region since an operand is dropped. I think its easier to do by cloning. So leaving as is.

mravishankar closed this revision.Oct 14 2020, 2:11 PM

Has been folded into commit rGde2568aab819: [mlir][Linalg] Rethink fusion of linalg ops with reshape ops. (authored by mravishankar)