Page MenuHomePhabricator

[mlir][affine] Fix unfolded bounding maps for affine.for
ClosedPublic

Authored by eopXD on Thu, Mar 25, 1:46 AM.

Details

Summary

Loop bounds of affine.for didn't perform foldings like affine.load, affine.store.
Bound maps shall be more composed, leaving most affine.apply become dead.

This resolves the bug listed on https://bugs.llvm.org/show_bug.cgi?id=45203

Diff Detail

Event Timeline

eopXD created this revision.Thu, Mar 25, 1:46 AM
eopXD requested review of this revision.Thu, Mar 25, 1:46 AM
eopXD edited the summary of this revision. (Show Details)Thu, Mar 25, 1:48 AM
eopXD edited the summary of this revision. (Show Details)
eopXD updated this revision to Diff 333239.Thu, Mar 25, 2:48 AM
This comment was removed by eopXD.
eopXD updated this revision to Diff 333242.Thu, Mar 25, 3:14 AM

revert to 1st commit of this bug fix.

eopXD updated this revision to Diff 333253.Thu, Mar 25, 4:05 AM

[mlir][affine][test] Check if affine.for has bounding maps composed

bondhugula requested changes to this revision.Thu, Mar 25, 7:03 AM

Thanks for improving this. A few comments.

mlir/test/Dialect/Affine/canonicalize.mlir
700

Nit: Terminate comment with period.
is -> are

703

No need of checking for module, but please add CHECK-LABEL's for each function.

704–709

Avoid numbered SSA values in test cases. If you don't want to capture them.

%arg1, %arg0 -> %{{.*}}

713

Please update this test case to cover lower bound composition as well.

717

New line on last line.

This revision now requires changes to proceed.Thu, Mar 25, 7:03 AM
eopXD updated this revision to Diff 333491.Thu, Mar 25, 9:13 PM

Adjust testcase to include lower bound composition,
also adjusted testcase to avoid numbered SSA.

Adjust testcase to include lower bound composition,
also adjusted testcase to avoid numbered SSA.

Please feel free to mark comments as "Done" if they are done.

eopXD marked 5 inline comments as done.Fri, Mar 26, 2:45 AM

Yes the comments are all done.
Thank you for the comments.

Loop bounds of affine.load

Typo in commit summary.

bondhugula accepted this revision.Thu, Apr 1, 8:34 PM

Looks good.

This revision is now accepted and ready to land.Thu, Apr 1, 8:34 PM
eopXD edited the summary of this revision. (Show Details)Fri, Apr 2, 8:43 AM

@eopXD Are you able to commit this? If not, I can commit it for you.

eopXD added a comment.Mon, Apr 5, 7:10 PM

@bondhugula I am not able to commit this.
Please help me to commit, thank you.

@bondhugula I am not able to commit this.
Please help me to commit, thank you.

Can you please rebase your commit on main and update this revision?

eopXD added a comment.Wed, Apr 7, 11:17 AM

Hi @bondhugula,
I am trying to rebase with $ git rebase origin/main with the current HEAD on my patching commit.
Then I encountered merge conflicts of file:

CONFLICT (content): Merge conflict in libunwind/CMakeLists.txt
CONFLICT (content): Merge conflict in libcxx/CMakeLists.txt

Is this normal or how shall I do something different to rebase to main?

bondhugula added a comment.EditedWed, Apr 7, 8:46 PM

Hi @bondhugula,
I am trying to rebase with $ git rebase origin/main with the current HEAD on my patching commit.
Then I encountered merge conflicts of file:

CONFLICT (content): Merge conflict in libunwind/CMakeLists.txt
CONFLICT (content): Merge conflict in libcxx/CMakeLists.txt

Is this normal or how shall I do something different to rebase to main?

A rebase leading to conflicts like these in unrelated files is indication that some developer/some process really messed up the LLVM git history upstream (@mehdi_amini @rriddle may have more insights here). In this case, what you'll have to do is to just cherry-pick your commit on the upstream main tip instead of rebasing. That will avoid all these conflicts completely unrelated to your PR. You will still see some conflicts on the affine ops files you are touching because you used a pretty old base - from at least a month ago. (Everyone using a significantly old base that traverses the time point where the history got messed up would see such conflcits while rebasing I believe.)

eopXD updated this revision to Diff 336728.Sun, Apr 11, 7:42 PM

Cherry-pick commit to upstream main.

eopXD updated this revision to Diff 336729.Sun, Apr 11, 7:47 PM

[mlir][affine] Fix unfolded bounding maps for affine.for

Loop bounds of affine.load didn’t perform foldings like affine.load, affine.store.
Bound maps shall be more composed, leaving most affine.apply become dead.

This resolves the bug listed on https://bugs.llvm.org/show_bug.cgi?id=45203
Differential Revision: https://reviews.llvm.org/D99323

This revision was automatically updated to reflect the committed changes.