Page MenuHomePhabricator

[MLIR] Fix fusion of linalg.indexed_generic producer into tiled (Indexed)GenericOp.
ClosedPublic

Authored by pifon2a on Apr 15 2020, 8:02 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Apr 15 2020, 8:02 AM
nicolasvasilache accepted this revision.Apr 15 2020, 8:23 AM

Looks great, thanks Alex!

mlir/test/Dialect/Linalg/fusion_indexed_generic.mlir
60

Can we add a

// CHECK-NOT:  addi

above this line?

This revision is now accepted and ready to land.Apr 15 2020, 8:23 AM
bondhugula requested changes to this revision.Apr 15 2020, 8:57 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
107

You don't need region().getBlocks().front(), region().front() will work.

This revision now requires changes to proceed.Apr 15 2020, 8:57 AM
bondhugula added inline comments.Apr 15 2020, 9:06 AM
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
114

Please add a comment here on the replacement.

115–119

Could you just use replaceAllUsesExcept(oldIndex, newIndex, {newIndexOp} for this entire six lines? It's just needs to be exposed.

ftynse accepted this revision.Apr 15 2020, 9:28 AM
ftynse added a subscriber: ftynse.

A slight concern on this being ultimately used within the pattern rewriting infrastructure that does interact well with brutal editing of uses. There are other aspects in this code that make me think it shouldn't be called from that infrastructure anyway, so not blocking.

pifon2a updated this revision to Diff 257991.Apr 16 2020, 1:41 AM

Addressed the comments and exposed replaceAllUsesExcept in LoopUtils.h.

pifon2a marked 4 inline comments as done.Apr 16 2020, 1:43 AM

Thanks Nicolas, Alex, Uday!

This revision was not accepted when it landed; it landed in state Needs Review.Apr 16 2020, 2:14 AM
This revision was automatically updated to reflect the committed changes.
bondhugula added inline comments.Apr 16 2020, 8:21 AM
mlir/include/mlir/Transforms/LoopUtils.h
290

Why is this added to LoopUtils.h? It has nothing to do with loops in particular. This could have been in Value.h.

ftynse added inline comments.Apr 16 2020, 9:11 AM
mlir/include/mlir/Transforms/LoopUtils.h
290

It sounds like moving that across can be a separate NFC change

pifon2a marked 3 inline comments as done.Apr 18 2020, 7:04 AM
pifon2a added inline comments.
mlir/include/mlir/Transforms/LoopUtils.h
290