Page MenuHomePhabricator

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

Authored by eopXD on Mar 25 2021, 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.Mar 25 2021, 1:46 AM
eopXD requested review of this revision.Mar 25 2021, 1:46 AM
eopXD edited the summary of this revision. (Show Details)Mar 25 2021, 1:48 AM
eopXD edited the summary of this revision. (Show Details)
eopXD updated this revision to Diff 333239.Mar 25 2021, 2:48 AM
This comment was removed by eopXD.
eopXD updated this revision to Diff 333242.Mar 25 2021, 3:14 AM

revert to 1st commit of this bug fix.

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

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

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

Thanks for improving this. A few comments.

mlir/test/Dialect/Affine/canonicalize.mlir
700 ↗(On Diff #333253)

Nit: Terminate comment with period.
is -> are

703 ↗(On Diff #333253)

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

704–709 ↗(On Diff #333253)

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

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

713 ↗(On Diff #333253)

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

718 ↗(On Diff #333253)

New line on last line.

This revision now requires changes to proceed.Mar 25 2021, 7:03 AM
eopXD updated this revision to Diff 333491.Mar 25 2021, 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.Mar 26 2021, 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.Apr 1 2021, 8:34 PM

Looks good.

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

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

eopXD added a comment.Apr 5 2021, 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.Apr 7 2021, 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.EditedApr 7 2021, 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.Apr 11 2021, 7:42 PM

Cherry-pick commit to upstream main.

eopXD updated this revision to Diff 336729.Apr 11 2021, 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.