This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][affine] Certain Call Ops to prevent fusion
ClosedPublic

Authored by vinayaka-polymage on Feb 22 2021, 9:14 PM.

Details

Summary

Fixes a bug in affine fusion pipeline where an incorrect fusion is performed
despite a Call Op that potentially modifies memrefs under consideration
exists between source and target.

Diff Detail

Event Timeline

vinayaka-polymage requested review of this revision.Feb 22 2021, 9:14 PM
dcaballe accepted this revision.Feb 22 2021, 9:59 PM

It looks great! Thank you so much!

This revision is now accepted and ready to land.Feb 22 2021, 9:59 PM
dcaballe added inline comments.Feb 22 2021, 11:32 PM
mlir/lib/Transforms/LoopFusion.cpp
774

Giving this another thought, should we be more conservative and add a node for any operation that uses a memref, even if it's unknown? In other words, we could remove this check and add a check like:

else if (llvm::any_of(op.getOperands(), [&](Value v) {
            return v.getType().isa<MemRefType>();
          })) {
                ...

after the MemoryEffectOpInterface check below. It would catch any CallOpInterface op and any other unknown operation using a memref.

mlir/lib/Transforms/LoopFusion.cpp
774

That sounds good. We might not even need checking MemoryEffectOpInterface then?

dcaballe added inline comments.Feb 23 2021, 9:36 AM
mlir/lib/Transforms/LoopFusion.cpp
774

That sounds good. We might not even need checking MemoryEffectOpInterface then?

That was my impression when looking at your first patch. Since this is a correctness issue, I think it's important to be overconservative and then add exceptions as we see fit.

bondhugula added inline comments.Feb 24 2021, 11:07 AM
mlir/lib/Transforms/LoopFusion.cpp
774

I don't think that would be the right approach. For eg., we can't be assuming that side-effect free ops using memrefs are writing to memrefs! For eg. if you just assumed any op could, you'd run into cases where things like dim op would start blocking fusion. It's not just overconservative but a limitation and not the right thing. If a registered op is really writing to memrefs or memory, it is wrong for the op to not declare the appropriate memory effects. If it doesn't declare those, it's not the transformation's responsibility to still work correctly. You can of course be conservative for unregistered ops perhaps. I believe a number of transformations/utilities make use of the effects interface and they rely on the ops correctly setting those effects. One of the reasons the effects interfaces exist is to not have to whitelist/hardcode a specific set of ops or not be forced to be overconservative.

dcaballe added inline comments.Feb 24 2021, 11:25 AM
mlir/lib/Transforms/LoopFusion.cpp
774

Ok, that sounds reasonable. I was more worried about ops with arbitrary side effects. I think unregistered ops would fall into that category, right? Anyways, we could add support for those cases in a separate patch. This patch should be ready to go. Thanks!

bondhugula added inline comments.Feb 24 2021, 8:14 PM
mlir/lib/Transforms/LoopFusion.cpp
774

That's right - I think we can check for unregistered ops and assume the most conservative case for it. For registered ops, I think we are fine with memory effects.

Thanks a lot for the insights @bondhugula and @dcaballe ! No more changes needed for this patch then.

bondhugula accepted this revision.Feb 25 2021, 9:55 PM

One minor comment.

mlir/lib/Transforms/LoopFusion.cpp
769

You could just use getOperandTypes() here.

Addressing the getOperandTypes comment

mlir/lib/Transforms/LoopFusion.cpp
769

Thanks for the tip! I have made the change.

bondhugula accepted this revision.Feb 26 2021, 1:49 AM
This revision was automatically updated to reflect the committed changes.