This is an archive of the discontinued LLVM Phabricator instance.

Differential D82158

[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

Reviewers
bondhugula
andydavis1
Commits
rG2b5d1776ffad: [MLIR][Affine-loop-fusion] Fix a bug in affine-loop-fusion pass when there are…
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

Unit TestsFailed

TimeTest
700 mslinux > MLIR.Transforms::Unknown Unit Message ("")
1,160 mswindows > MLIR.Transforms::Unknown Unit Message ("")

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
Herald added subscribers: msifontes, jurahul, Kayjukh and 14 others. · View Herald Transcript
tungld updated this revision to Diff 271954.Jun 19 2020, 1:40 AM

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

ftynse added reviewers: bondhugula, andydavis1.Jun 19 2020, 1:52 AM
Harbormaster failed remote builds in B60959: Diff 271951!Jun 19 2020, 2:07 AM
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 ↗(On Diff #271954)

Change to /// comments please.

966–969 ↗(On Diff #271954)

Avoid auto here for readability please.

969 ↗(On Diff #271954)

Comment here please on what this iteration is doing.

974 ↗(On Diff #271954)

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

993 ↗(On Diff #271954)

/// comments please.

994 ↗(On Diff #271954)

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 ↗(On Diff #271954)

Comment fix.

1004 ↗(On Diff #271954)

Aren't you only interested in memref values here?

1004–1006 ↗(On Diff #271954)

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

1011 ↗(On Diff #271954)

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

1870 ↗(On Diff #271954)

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.

Harbormaster failed remote builds in B61177: Diff 272325!Jun 21 2020, 7:42 PM
tungld added a comment.Jun 21 2020, 7:45 PM

@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 ↗(On Diff #272331)

Avoid auto here please.

984 ↗(On Diff #272331)

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.

tungld added a comment.Jun 23 2020, 9:35 PM

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

bondhugula added a comment.Jun 24 2020, 9:41 AM
In D82158#2110621, @tungld wrote:

@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 added a comment.Jun 24 2020, 9:58 AM
In D82158#2111900, @bondhugula wrote:
In D82158#2110621, @tungld wrote:

@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?

tungld added a comment.Jun 24 2020, 5:35 PM

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

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

Closed by commit rG2b5d1776ffad: [MLIR][Affine-loop-fusion] Fix a bug in affine-loop-fusion pass when there are… (authored by tungld, committed by bondhugula). · Explain WhyJun 26 2020, 5:58 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
mlir/
test/
Transforms/
64 lines

Diff 271951

mlir/test/Transforms/loop-fusion.mlir

Show First 20 Lines • Show All 2,529 Lines • ▼ Show 20 Lines
// CHECK-NOT: affine.for // CHECK-NOT: affine.for
// CHECK: addf // CHECK: addf
// CHECK-NOT: affine.for // CHECK-NOT: affine.for
// CHECK: subf // CHECK: subf
// CHECK-NOT: affine.for // CHECK-NOT: affine.for
// CHECK: mulf // CHECK: mulf
// CHECK-NOT: affine.for // CHECK-NOT: affine.for
// CHECK: divf // CHECK: divf
// -----
// CHECK-LABEL: func @should_not_fuse_since_non_affine_users
func @should_not_fuse_since_non_affine_users(%in0 : memref<32xf32>,
%in1 : memref<32xf32>) {
affine.for %d = 0 to 32 {
%lhs = affine.load %in0[%d] : memref<32xf32>
%rhs = affine.load %in1[%d] : memref<32xf32>
%add = addf %lhs, %rhs : f32
affine.store %add, %in0[%d] : memref<32xf32>
}
affine.for %d = 0 to 32 {
%lhs = load %in0[%d] : memref<32xf32>
%rhs = load %in1[%d] : memref<32xf32>
%add = subf %lhs, %rhs : f32
store %add, %in0[%d] : memref<32xf32>
}
affine.for %d = 0 to 32 {
%lhs = affine.load %in0[%d] : memref<32xf32>
%rhs = affine.load %in1[%d] : memref<32xf32>
%add = mulf %lhs, %rhs : f32
affine.store %add, %in0[%d] : memref<32xf32>
}
return
}
// CHECK: affine.for
// CHECK: addf
// CHECK: affine.for
// CHECK: subf
// CHECK: affine.for
// CHECK: mulf
// -----
// CHECK-LABEL: func @should_not_fuse_since_top_level_non_affine_users
func @should_not_fuse_since_top_level_non_affine_users(%in0 : memref<32xf32>,
%in1 : memref<32xf32>) {
%sum = alloc() : memref<f32>
affine.for %d = 0 to 32 {
%lhs = affine.load %in0[%d] : memref<32xf32>
%rhs = affine.load %in1[%d] : memref<32xf32>
%add = addf %lhs, %rhs : f32
store %add, %sum[] : memref<f32>
affine.store %add, %in0[%d] : memref<32xf32>
}
%load_sum = load %sum[] : memref<f32>
affine.for %d = 0 to 32 {
%lhs = affine.load %in0[%d] : memref<32xf32>
%rhs = affine.load %in1[%d] : memref<32xf32>
%add = mulf %lhs, %rhs : f32
%sub = subf %add, %load_sum: f32
affine.store %sub, %in0[%d] : memref<32xf32>
}
dealloc %sum : memref<f32>
return
}
// CHECK: affine.for
// CHECK: addf
// CHECK: affine.for
// CHECK: mulf
// CHECK: subf