Page MenuHomePhabricator

[MLIR][affine] Prevent fusion when ops with memory effect free are present between producer and consumer
ClosedPublic

Authored by vinayaka-polymage on Feb 18 2021, 9:49 PM.

Details

Summary

This commit fixes a bug in affine fusion pipeline where an
incorrect fusion is performed despite a dealloc op is present
between a producer and a consumer. This is done by creating a
node for dealloc op in the MDG.

Diff Detail

Event Timeline

vinayaka-polymage requested review of this revision.Feb 18 2021, 9:49 PM
dcaballe requested changes to this revision.Feb 19 2021, 10:49 AM

Hey Vinayaka!

Thank you so much for the fix! Some comments below.

mlir/lib/Transforms/LoopFusion.cpp
772

This looks good to me! I think we have to be more conservative and also add any non-affine operation that produces or uses a memref. Otherwise, hasNonAffineUsersOnThePath won't return the expected output if we have a read, call or alloc-like op or any unknown operation that could potentially make the memref escape, right?

mlir/test/Transforms/loop-fusion.mlir
2849

Would you mind adding also tests with a call op, a load op and an unknown op using the memref, if we don't have it already? Thanks!

2860

I think we should check loads and stores also because we may fuse and do not remove the producer loop.

This revision now requires changes to proceed.Feb 19 2021, 10:49 AM

https://reviews.llvm.org/D97030 is somehow related. I think the common solution is to represent these missing dependences in the MDG. We may have to coordinate the fixes.

bondhugula accepted this revision.Feb 20 2021, 8:30 AM

LGTM.

mlir/lib/Transforms/LoopFusion.cpp
772

I feel those can be handled in a next patch. This doesn't fix everything but is catching writing and freeing users the right way.

mlir/test/Transforms/loop-fusion.mlir
2849

But this patch isn't meant to fix those. It's only for the users which have a write side-effect or a free side-effect.

dcaballe accepted this revision.Feb 21 2021, 10:27 PM

If you could improve the lit test so that loads and stores are also checked, that should be all. Thanks!

mlir/lib/Transforms/LoopFusion.cpp
772

Ok, fair enough!

This revision is now accepted and ready to land.Feb 21 2021, 10:27 PM
mlir/lib/Transforms/LoopFusion.cpp
772

Thanks for the suggestions!
I will create a new patch with the above suggested changes and paste the link here.

mlir/test/Transforms/loop-fusion.mlir
2860

That's correct, I had missed this, thanks! I have corrected this.

bondhugula accepted this revision.Feb 22 2021, 7:57 AM
dcaballe accepted this revision.Feb 22 2021, 9:39 AM

Thanks for addressing the comments!

This revision was landed with ongoing or failed builds.Feb 22 2021, 9:51 AM
This revision was automatically updated to reflect the committed changes.

Suggested additional checks and test-cases submitted at https://reviews.llvm.org/D97252