Page MenuHomePhabricator

[GVN] Improve analysis for missed optimization remark
AcceptedPublic

Authored by hnrklssn on Apr 29 2020, 8:53 AM.

Details

Reviewers
anemet
hfinkel
Summary

This change tries to handle multiple dominating users of the pointer operand
by choosing the most immediately dominating one, if possible.
While making this change I also found that the previous implementation
had a missing break statement, making all loads with an odd number of
dominating users emit an OtherAccess value, so that has also been fixed.

Diff Detail

Event Timeline

hnrklssn created this revision.Apr 29 2020, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 8:53 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Thanks for improving this. Can you please add a small test highlighting where your change improve the diagnostics? TIA!

hnrklssn updated this revision to Diff 261628.May 2 2020, 3:36 AM

[GVN] Update analysis for optimization remark and add tests

We now cover even more cases for finding OtherAccess.

This is my first time using both Phabricator and Arcanist, so I'm not entirely familiar with the UI and workflow, but I can't seem to find one of the test cases I added in my update. It's supposed to be in llvm/test/Transforms/GVN/opt-remarks-multiple-users.ll, but I can't see it added in the revision. Did I miss something? It's an earlier commit than the new diff I pushed, so I would've thought it would be included as well.

hnrklssn updated this revision to Diff 261629.May 2 2020, 4:31 AM

[GVN] [Remarks] Trying to push missing file

hnrklssn updated this revision to Diff 261630.May 2 2020, 4:34 AM

Updating D79097: [GVN] Improve analysis for missed optimization remark

Now all patches should be included... I think.

hnrklssn updated this revision to Diff 261637.May 2 2020, 5:22 AM

Updating D79097: reformatting code according to pre-merge check

Sorry about the delay, just a few small nits on the tests, this is pretty close. Thanks for pushing this!

llvm/test/Transforms/GVN/opt-remarks-multiple-users.ll
3 ↗(On Diff #261637)

Please add a comment with the intention for test. In this particular case I imagine that before your change we would have given up on determining OtherAccess.

44 ↗(On Diff #261637)

I know this is a minimized testcase but don’t have unreachable here to make it more realistic.

llvm/test/Transforms/GVN/opt-remarks-non-dominating.ll
61 ↗(On Diff #261637)

Please add a comment before each functions what it’s testing.

63 ↗(On Diff #261637)

I would change these undefs to actual values to make these tests more realistic.

@hnrklssn hi, are you panning to get back to this at some point?

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

Yup, it will be published in LUP Student Papers afaict! I can send you a rough draft if you're interested. Most of the final content is in place already, but it's a bit rough around the edges.

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

Yup, it will be published in LUP Student Papers afaict! I can send you a rough draft if you're interested. Most of the final content is in place already, but it's a bit rough around the edges.

No hurry, the final version is fine. You may also want to consider sending it to llvm-dev if you think it could be interesting to other people in the community.

hnrklssn updated this revision to Diff 338920.EditedTue, Apr 20, 10:42 AM

Addressed review comments and fixed a crash for non-local variables.

hnrklssn marked 3 inline comments as done.Tue, Apr 20, 10:45 AM

After a long wait I'm now finally able to work on this again! Please excuse the delay.

llvm/test/Transforms/GVN/opt-remarks-non-dominating.ll
63 ↗(On Diff #261637)

Missed this comment, will update again!

hnrklssn updated this revision to Diff 338931.Tue, Apr 20, 10:55 AM

Removed undefs in test cases.

hnrklssn marked an inline comment as done.Tue, Apr 20, 10:57 AM

It should be ready to review again now :)

Glad, you were able to get back to this.

llvm/test/Transforms/GVN/opt-remarks-multiple-users.ll
15 ↗(On Diff #338931)

It seems that we want to be able to distinguish between the two calls here?

If you add source debug info to test along the lines of GVN/opt-remarks.ll you should be able to check that the right call is returned here.

Also a nit, you may want to chop up the CHECK section so that each checking section is right before the function it corresponds too.

hnrklssn updated this revision to Diff 339788.Thu, Apr 22, 2:53 PM

Split CHECKs to keep them closer to their relevant functions, and added debug information to disambiguate instructions where relevant.

hnrklssn marked an inline comment as done.Thu, Apr 22, 2:53 PM
hnrklssn updated this revision to Diff 339910.Fri, Apr 23, 12:20 AM

Fixed formatting issue

anemet accepted this revision.Thu, May 6, 4:15 PM

Looks great, thanks!

This revision is now accepted and ready to land.Thu, May 6, 4:15 PM

Cool :) This is my first submission, so I don't have commit access. If I understand correctly that means I need someone else to commit it on my behalf, yes?

anemet added a comment.Fri, May 7, 8:35 AM

Congrats ;). Yes, I will commit it for you. Can you please rebase and retest, looks like the patch is not applying cleanly.

hnrklssn updated this revision to Diff 343770.Fri, May 7, 3:06 PM

Rebasing to master

Forgot to rebase onto upstream... That explains why it went so smoothly!

hnrklssn updated this revision to Diff 343828.Sat, May 8, 3:58 AM

Rebased onto upstream/main

hnrklssn updated this revision to Diff 343833.Sat, May 8, 5:50 AM

Ran clang-format