search

tensorflow / mlir

"Multi-Level Intermediate Representation" Compiler Infrastructure
1.74k stars 260 forks source link

AffineLoopFusion: Prevent fusion of multi-out-edge producer loops #259

Closed dcaballe closed 4 years ago

dcaballe commented 5 years ago

In https://github.com/tensorflow/mlir/pull/162 I introduced a bug that incorrectly allowed fusion of producer loops with multiple outgoing edges. This commit fixes that problem. It also introduces a new flag to disable sibling loop fusion so that we can test producer-consumer fusion in isolation.

dcaballe commented 5 years ago

Thanks for the comments, @bondhugula! I can't add reviewers to MLIR PRs probably because I don't have permission. Maybe @andydavis1 is also interested?

andydavis1 commented 5 years ago

Hi Diego, thanks for contributing! I can take a look at this...

bondhugula commented 5 years ago

Modified original test and removed clSiblingFusion. I also added a new test (and fixes related to it) that it asserted now. It’s a producer with multiple outgoing edges where there is a single store with a single outgoing edge.

Please, let me know if you have any other comments. Thanks!

I'm a bit confused here. Why should the fusion be prevented here in cases like these where it's valid (referring to the test case you added) - where you have a single producer and multiple consumers? I recall this was what you had addressed in your earlier commit. Are you introducing the limitation back? :-)

dcaballe commented 5 years ago

I'm a bit confused here. Why should the fusion be prevented here in cases like these where it's valid (referring to the test case you added) - where you have a single producer and multiple consumers? I recall this was what you had addressed in your earlier commit. Are you introducing the limitation back? :-)

My previous commit introduced support for multi-store producers with a single outgoing edge within the function. That is still working fine. The test I added is intact: https://github.com/tensorflow/mlir/pull/259/files/b8e7f532a846e8a4f76b04473db21e9ffb7d15f7#diff-ea810dda41c98feaab3d122f33648fb5R2327. However, as part of that change, I asserted on that producers reaching canFuseSrcWhichWritesToLiveOut should have only one outgoing edge (https://github.com/tensorflow/mlir/pull/162/files#diff-5b47f3417e354b0773b092edc5d8740cR1014). That is not true because a single store may have multiple outgoing edges and loads can also have outgoing edges. That is the piece I’m reverting and adding test for it, which is a bit complicated due to the producer-consumer fusion and sibling fusion interaction.

Does it make sense now? :)

bondhugula commented 5 years ago

I'm a bit confused here. Why should the fusion be prevented here in cases like these where it's valid (referring to the test case you added) - where you have a single producer and multiple consumers? I recall this was what you had addressed in your earlier commit. Are you introducing the limitation back? :-)

My previous commit introduced support for multi-store producers with a single outgoing edge within the function. That is still working fine. The test I added is intact: https://github.com/tensorflow/mlir/pull/259/files/b8e7f532a846e8a4f76b04473db21e9ffb7d15f7#diff-ea810dda41c98feaab3d122f33648fb5R2327. However, as part of that change, I asserted on that producers reaching canFuseSrcWhichWritesToLiveOut should have only one outgoing edge (https://github.com/tensorflow/mlir/pull/162/files#diff-5b47f3417e354b0773b092edc5d8740cR1014). That is not true because a single store may have multiple outgoing edges and loads can also have outgoing edges. That is the piece I’m reverting and adding test for it, which is a bit complicated due to the producer-consumer fusion and sibling fusion interaction.

Does it make sense now? :)

I see - sounds good then.