This is an archive of the discontinued LLVM Phabricator instance.

[GMR] Be a bit smarter about which globals alias when doing recursive lookups
ClosedPublic

Authored by mkuper on Aug 10 2015, 5:30 AM.

Details

Summary

This should hopefully fix PR24288 even harder.

To be honest, I'm not sure this is the right condition.
Does this look sane? That is, is the zero-size check really necessary (and is the way I'm checking it correct)?

Diff Detail

Event Timeline

mkuper updated this revision to Diff 31657.Aug 10 2015, 5:30 AM
mkuper retitled this revision from to [GMR] Be a bit smarter about which globals alias when doing recursive lookups.
mkuper updated this object.
mkuper added reviewers: chandlerc, majnemer.
mkuper added a subscriber: llvm-commits.
majnemer added inline comments.Aug 10 2015, 6:31 AM
lib/Analysis/IPA/GlobalsModRef.cpp
720–729

I guess a zero-sized global can never be "modified" as you would *actually* modify some other global (or commit UB if there was no other global). However, it's possible to have a global variable in one TU and a global alias in another TU with the same name which makes me think that you need to check that the linkage is not mayBeOverridden.

mkuper updated this revision to Diff 31664.Aug 10 2015, 6:58 AM

Thanks, David!

Added the check mayBeOverwritten() check.

About the zero-size check, what if you have two consecutive zero-size globals, followed by a third (non-zero-size) global? (That sounds like nonsense, but I don't quite understand the semantics of zero-size globals to begin with.)

chandlerc accepted this revision.Aug 10 2015, 11:32 AM
chandlerc edited edge metadata.

LGTM with nit picks below.

lib/Analysis/IPA/GlobalsModRef.cpp
720–732

"... never alias, unless overridden or zero-sized."

728–729

I would explicitly write "> 0" for these.

This revision is now accepted and ready to land.Aug 10 2015, 11:32 AM
majnemer added inline comments.Aug 10 2015, 11:37 AM
lib/Analysis/IPA/GlobalsModRef.cpp
728–729

Note that the type might not be sized if it's a global variable with an opaque body. You might want to query isSized first.

This revision was automatically updated to reflect the committed changes.