Page MenuHomePhabricator

Let GetUnderlyingObject/Objects use MemorySSA
AbandonedPublic

Authored by aqjune on Aug 4 2018, 2:22 AM.

Details

Reviewers
None
Summary

This patch makes GetUnderlyingObject/Objects use MemorySSA if it is given.

Any comment is welcome. :)

Diff Detail

Event Timeline

aqjune created this revision.Aug 4 2018, 2:22 AM
hfinkel added a subscriber: hfinkel.Aug 4 2018, 5:47 AM

Seems like an interesting idea. What's the motivation for doing this?

I'm uneasy with making GetUnderlyingObject fundamentally more powerful than GetUnderlyingObjects - the latter should always provide a superset of the functionality of the former.

lib/Analysis/ValueTracking.cpp
3512

You can use dyn_cast_or_null here:

if (StoreInst *DefI = dyn_cast_or_null<StoreInst>(MDef->getMemoryInst())) {

Thanks for this! In addition to Hal's comments...

lib/Analysis/ValueTracking.cpp
3500

This isn't always true: MSSA models volatile/atomic (>= monotonic) loads as MemoryDefs. Do we check for this elsewhere in this function?

If so, all of this can be collapsed into auto *MLoad = cast<MemoryUse>(MSSA->getMemoryAccess(LI));. If not, please make it a dyn_cast<MemoryUse>(MSSA->getMemoryAccess(LI)) + null check.

3511

Nit: I think it's way more common/somewhat more readable to say MDef != MSSA->getLiveOnEntry().

If any other Def/Use has a null MemoryInst, MSSA is probably broken. :)

aqjune updated this revision to Diff 159196.Aug 4 2018, 7:30 PM
aqjune retitled this revision from Let GetUnderlyingObject use MemorySSA to Let GetUnderlyingObject/Objects use MemorySSA.
aqjune edited the summary of this revision. (Show Details)
  • Address comments
  • Update the title
  • Add a new test that deals with load volatile
aqjune added a comment.EditedAug 4 2018, 7:35 PM

Thanks for the comments!

@hfinkel The motivation is that GetUnderlyingObject stucks at cases like this:

store i8* %p, i8** %q
%p2 = load i8*, i8** %q ; p2 is p

If we have MemorySSA, this can be resolved in constant time. I believe BasicAliasAnalysis can get benefit this, hence supporting partially flow sensitive analysis.

Regarding GetUnderlyingObjects - this patch contains update to help GetUnderlyingObjects use MemorySSA as well, by passing the argument into GetUnderlyingObject. I found that description/title of this patch wasn't containing the info. so I updated it.

aqjune marked 2 inline comments as done.Aug 4 2018, 7:36 PM
aqjune marked an inline comment as done.

Thanks for the comments!

@hfinkel The motivation is that GetUnderlyingObject stucks at cases like this:

store i8* %p, i8** %q
%p2 = load i8*, i8** %q ; p2 is p

If we have MemorySSA, this can be resolved in constant time. I believe BasicAliasAnalysis can get benefit this, hence supporting partially flow sensitive analysis.

I read that in the comment ;) -- but why? Shouldn't all such cases be simplified by GVN (etc.) into a form where GetUnderlyingObjects, as is, can reason about them?

Regarding GetUnderlyingObjects - this patch contains update to help GetUnderlyingObjects use MemorySSA as well, by passing the argument into GetUnderlyingObject. I found that description/title of this patch wasn't containing the info. so I updated it.

aqjune added a comment.Aug 6 2018, 1:54 AM
store i8* %p, i8** %q
%p2 = load i8*, i8** %q ; p2 is p

If we have MemorySSA, this can be resolved in constant time. I believe BasicAliasAnalysis can get benefit this, hence supporting partially flow sensitive analysis.

I read that in the comment ;) -- but why? Shouldn't all such cases be simplified by GVN (etc.) into a form where GetUnderlyingObjects, as is, can reason about them?

Ideally, yes - but wouldn't eagerly removing such store-load pairs be costly?
A pair of store-load still remains in this example as well - https://godbolt.org/g/zutKyi (load i8*, i8** still remains)
Or I can search for a way to remove such pairs in more eager way, by adopting MemorySSA into InstSimplify or making GVN remove the pair in the example above.

jdoerfert added inline comments.
lib/Analysis/ValueTracking.cpp
3512

Couldn't the store write to more than just the values read by the load?
If so, do we need to take that into account?

aqjune abandoned this revision.Dec 5 2019, 12:28 PM