This is an archive of the discontinued LLVM Phabricator instance.

[MemorySSA] Make the use of moveAllAfterMergeBlocks consistent.
ClosedPublic

Authored by asbirlea on Oct 8 2019, 12:16 PM.

Details

Summary

The rule for the moveAllAfterMergeBlocks API si for all instructions
from From to have been moved to To, while keeping the CFG edges (and
block terminators) unchanged.
Update all the callsites for moveAllAfterMergeBlocks to follow this.

Pending follow-up: since the same behavior is needed everytime, merge
all callsites into one. The common denominator may be the call to
MergeBlockIntoPredecessor.

Resolves PR43569.

Diff Detail

Event Timeline

asbirlea created this revision.Oct 8 2019, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2019, 12:16 PM
george.burgess.iv accepted this revision.Oct 8 2019, 8:46 PM

Thanks for this!

LGTM with a few nits

lib/Analysis/MemorySSAUpdater.cpp
1185 ↗(On Diff #223920)

nit: !Defs->empty()?

(Though I'm surprised this check is required -- shouldn't we be removing entries from the map as they become empty?)

1186 ↗(On Diff #223920)

If I'm reading the description properly, this function assumes that everything in From is now in To, no?

If so, cast<MemoryPhi>() seems more appropriate here, since the only thing that can remain is a single Phi

test/Analysis/MemorySSA/pr43569.ll
1 ↗(On Diff #223920)

; REQUIRES: asserts?

This revision is now accepted and ready to land.Oct 8 2019, 8:46 PM
asbirlea updated this revision to Diff 224068.Oct 9 2019, 8:48 AM
asbirlea marked 4 inline comments as done.

Address comments.

lib/Analysis/MemorySSAUpdater.cpp
1186 ↗(On Diff #223920)

This method (moveAllAccesses) is also called from moveAllAfterSpliceBlocks, when From is not necessarily empty. I'll update the comment above.

This revision was automatically updated to reflect the committed changes.