Page MenuHomePhabricator

[MLIR] Fix affine fusion bug/efficiency issue / enable more fusion
ClosedPublic

Authored by bondhugula on May 6 2020, 8:55 PM.

Details

Summary

The list of destination load ops while evaluating producer-consumer
fusion wasn't being maintained as a set, and as such, duplicate load ops
were being added to it. Although this is harmless correctness-wise, it's
a killer efficiency-wise and it prevents interesting/useful fusions
(including for eg. reshapes into a matmul). The reason the latter
fusions would be missed is that a slice union would be unnecessarily
needed due to the duplicate load ops on a memref added to the 'dst
loads' list. Since slice union is unimplemented for the local var case,
a single destination load op that leads to local vars (like a floordiv /
mod producing fusion), a common case, would not get fused due to an
unnecessary union being tried with itself. (The union would actually be
the same thing but we would bail out.)

Besides the above, this would also significantly speed up fusion as all
the unnecessary slice computations / unions, checks, etc. due to the
duplicates go away.

Diff Detail

Event Timeline

bondhugula created this revision.May 6 2020, 8:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2020, 8:55 PM
andydavis1 accepted this revision.May 6 2020, 9:25 PM

Thanks Uday! Looks good...

mlir/lib/Transforms/LoopFusion.cpp
1628–1631

Is this a TODO: consider using a hash set if performance becomes an issue?

This revision is now accepted and ready to land.May 6 2020, 9:25 PM
bondhugula marked 2 inline comments as done.May 6 2020, 10:10 PM
bondhugula added inline comments.
mlir/lib/Transforms/LoopFusion.cpp
1628–1631

It's not really a TODO at this point. A vector should be fine unless there are use cases where this is shown to be a bottleneck. We are only going to have a handful of ops, and if we have hundreds for some use case, there'd also other places to address.

This revision was automatically updated to reflect the committed changes.
bondhugula marked an inline comment as done.

LGTM, Thanks!