This is an archive of the discontinued LLVM Phabricator instance.

[MemorySSA] Restrict optimizations after a PhiTranslation.
ClosedPublic

Authored by asbirlea on Jul 29 2020, 7:05 PM.

Details

Summary

Merging alias results from different paths, when a path did phi
translation is not necesarily correct. Conservatively terminate such paths.
Aimed to fix PR46156.

Diff Detail

Event Timeline

asbirlea created this revision.Jul 29 2020, 7:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2020, 7:05 PM
asbirlea requested review of this revision.Jul 29 2020, 7:05 PM
fhahn added inline comments.Jul 30 2020, 11:02 AM
llvm/lib/Analysis/MemorySSA.cpp
528

might be worth adding a comment, describing what it means (and when it will be reset).

Also, would it make sense to move this to a datastructure already tied to a set of paths that are explored together, e.g. the UpwardsMemoryQuery? The current approach seems fine to me, I am just wondering if we could reduce the scope of the variable a bit (and avoid relying on the reset calls).

asbirlea added inline comments.Jul 30 2020, 7:32 PM
llvm/lib/Analysis/MemorySSA.cpp
528

I believe the current approach does narrow the scope. It's reseting the state after optimizing towards a phi, which is the join point where we need to stop optimizing in the presence of a phi translation. The UpwardsMemoryQuery contains info about the query itself, and it is set at the start of the search (in findClobber). The scope of the bool needs to be similar to that of VisitedPhis, that of internal bookkeeping for a search, and very much related to phi optimization.

I'll add a comment to clarify.

asbirlea updated this revision to Diff 282108.Jul 30 2020, 7:42 PM

Add comments.

fhahn accepted this revision.Aug 3 2020, 11:17 AM

LGTM, thanks for adding he comments.

llvm/lib/Analysis/MemorySSA.cpp
528

Thanks, agreed it seems in line with the way caching/invalidation is done throughout here and changing that would require bigger changes without obvious benefit.

This revision is now accepted and ready to land.Aug 3 2020, 11:17 AM
george.burgess.iv accepted this revision.Aug 3 2020, 1:07 PM

lgtm with a few tiny nits. thanks for working on this!

llvm/include/llvm/Analysis/MemorySSA.h
1245–1246

super tiny nit: can we bring this field below DT so we save padding in this struct's layout?

llvm/lib/Analysis/MemorySSA.cpp
530

nit: referencing the PR might help add useful context

asbirlea updated this revision to Diff 282731.Aug 3 2020, 2:48 PM
asbirlea marked 4 inline comments as done.

Address comments.

This revision was landed with ongoing or failed builds.Aug 3 2020, 2:49 PM
This revision was automatically updated to reflect the committed changes.
fhahn added inline comments.Sep 14 2020, 8:46 AM
llvm/lib/Analysis/MemorySSA.cpp
612

I think currently we only set PerfomedPhiTranslation to true if we preformed phi translation for the first visited upwards def (we only check the status for the iterator of the first def, which will be copied and only the copies will be updated). But we could also perform phi translation for other defs we visit in with the iterator.

I think this is causing https://bugs.llvm.org/show_bug.cgi?id=47498 and I think we should set PerfomedPhiTranslation to true, if we do phi translations for any def visited in the iterator. I went ahead and pushed c4f1b3144184e4c276a7e7c801cbcd4ac3c573ba which changes the iterator to update a pointer to a bool to unblock a test-suite failure on PPC. I would appreciate if you could take a quick look to confirm this is indeed the expected behavior and I did not miss anything. Thanks!

asbirlea added inline comments.Sep 14 2020, 11:51 AM
llvm/lib/Analysis/MemorySSA.cpp
612

You're right, this was missing the bools set by the internal iterations. Thank you for the fix.