This is an archive of the discontinued LLVM Phabricator instance.

[GMR] Teach the conservative path of GMR to catch even more easy cases.
ClosedPublic

Authored by chandlerc on Aug 2 2015, 1:21 AM.

Details

Summary

In PR24288 it was pointed out that the easy case of a non-escaping
global and something that *obviously* required an escape sometimes is
hidden behind PHIs (or selects in theory). Because we have this binary
test, we can easily just check that all possible input values satisfy
the requirement. This is done with a (very small) recursion through PHIs
and selects. With this, the specific example from the PR is correctly
folded by GVN.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc updated this revision to Diff 31199.Aug 2 2015, 1:21 AM
chandlerc retitled this revision from to [GMR] Teach the conservative path of GMR to catch even more easy cases..
chandlerc updated this object.
chandlerc added reviewers: mkuper, hfinkel.
chandlerc added a subscriber: llvm-commits.
majnemer added inline comments.
lib/Analysis/IPA/GlobalsModRef.cpp
698 ↗(On Diff #31199)

Where is GV used?

729–730 ↗(On Diff #31199)

This comment could probably go on one line.

mkuper added inline comments.Aug 2 2015, 2:00 AM
lib/Analysis/IPA/GlobalsModRef.cpp
734 ↗(On Diff #31199)

colud -> could

762 ↗(On Diff #31199)

It looks like this also covers the case where we end up reaching GV itself in the recursion. Perhaps make that explicit?

806 ↗(On Diff #31199)

This looks a bit confusing.
Can we turn this into the more standard "if (!GV1) swap..." pattern?

chandlerc updated this revision to Diff 31214.Aug 2 2015, 9:40 PM
chandlerc marked 4 inline comments as done.

Updating patch based on reviewer comments.

Updated, and responses to review comments below...

lib/Analysis/IPA/GlobalsModRef.cpp
698 ↗(On Diff #31199)

Sorry, I was trying to use GV to do some stuff that ended up with the FIXME comment instead. =] Based on Michael's comment, I'll add in the stub that uses it around the FIXME comment, which should also make it much more clear how to safely implement that FIXME.

762 ↗(On Diff #31199)

Yea, that also gives me a much better place to anchor the FIXME above. Should make sure that we don't introduce a bug about the same GV when addressing that FIXME.

806 ↗(On Diff #31199)

I really don't like the swap pattern... Does doing this in separate variable initializers make it more clear? (Patch updated to do that...)

mkuper accepted this revision.Aug 2 2015, 10:15 PM
mkuper edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 2 2015, 10:15 PM
This revision was automatically updated to reflect the committed changes.