This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Affine-loop-fusion] Fix a bug in affine-loop-fusion pass when there are non-affine operations
ClosedPublic

Authored by tungld on Jun 19 2020, 1:26 AM.

Details

Summary

When there is a mix of affine load/store and non-affine operations (e.g. std.load, std.store),
affine-loop-fusion ignores the present of non-affine ops, thus changing the program semantics.

E.g. we have a program of three affine loops operating on the same memref in which one of them uses std.load and std.store, as follows.

affine.for
  affine.store %1
affine.for
  std.load %1
  std.store %1
affine.for
  affine.load %1
  affine.store %1

affine-loop-fusion will produce the following result which changed the program semantics:

affine.for
  std.load %1
  std.store %1
affine.for
  affine.store %1
  affine.load %1
  affine.store %1

This patch is to fix the above problem by checking non-affine users of the memref that are between the source and destination nodes of interest.

Diff Detail

Event Timeline

tungld created this revision.Jun 19 2020, 1:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2020, 1:26 AM
tungld updated this revision to Diff 271954.Jun 19 2020, 1:40 AM

Add missing changes for mlir/lib/Transforms/LoopFusion.cpp

bondhugula requested changes to this revision.Jun 20 2020, 5:56 AM

Thanks very much for addressing this issue. Some initial comments.

mlir/lib/Transforms/LoopFusion.cpp
961

Change to /// comments please.

966–969

Avoid auto here for readability please.

969

Comment here please on what this iteration is doing.

974

You don't need to use the found flag. Instead use .wasInterrupted() on walk to check if the walk was interrupted.

993

/// comments please.

994

Nit: mention whether between is inclusive or exclusive of srcId and dstId

This revision now requires changes to proceed.Jun 20 2020, 5:56 AM
bondhugula added inline comments.Jun 21 2020, 12:55 PM
mlir/lib/Transforms/LoopFusion.cpp
1001

Comment fix.

1004

Aren't you only interested in memref values here?

1004–1006

Use a SmallDenseSet? This will be quite expensive when nodes become larger.

1011

Instead of iterating over all values - iterate over only the memref values?

1872

non-affine memref access instead of non-affine operation?

tungld updated this revision to Diff 272325.Jun 21 2020, 7:40 PM

Update the code according to Uday Bondhugula's comments.

@bondhugula Thank you for your comments! I updated the code based on your comments.

tungld updated this revision to Diff 272331.Jun 21 2020, 9:55 PM

Add unit tests.

andydavis1 accepted this revision.Jun 22 2020, 10:55 AM

Thanks for addressing this issue! Let's see if Uday has any more comments before committing, but your latest changes look good to me...

bondhugula accepted this revision.Jun 23 2020, 12:47 PM

Looks good, thanks. A few more comments to address.

mlir/lib/Transforms/LoopFusion.cpp
969

Avoid auto here please.

984

You can instead use: llvm::is_contained(users, user) and also drop the trivial braces around.

This revision is now accepted and ready to land.Jun 23 2020, 12:47 PM
tungld updated this revision to Diff 272881.Jun 23 2020, 6:01 PM

Thanks Uday! I updated the code based on your comments.

bondhugula accepted this revision.Jun 23 2020, 8:08 PM

Thanks, please do commit. Let me know if you need me to.

@bondhugula it seems I don't have permission to push a commit. Could you please help me to land this patch? Many thanks!

@bondhugula it seems I don't have permission to push a commit. Could you please help me to land this patch? Many thanks!

Sure, will commit in a few minutes.

@bondhugula it seems I don't have permission to push a commit. Could you please help me to land this patch? Many thanks!

Sure, will commit in a few minutes.

Could you let me know what "name <email>" info you want on the commit?

Could you let me know what "name <email>" info you want on the commit?

Please use "Tung D. Le <tung@jp.ibm.com>". Thanks!

This revision was automatically updated to reflect the committed changes.