This is an archive of the discontinued LLVM Phabricator instance.

[MemorySSA] Update MemoryPhi wiring for block splitting to consider if identical edges were merged.
ClosedPublic

Authored by asbirlea on Aug 31 2018, 5:11 PM.

Details

Summary

Block splitting is done with either identical edges being merged, or not.
Only critical edges can be split without merging identical edges based on an option.
Teach the memoryssa updater to take this into account: for the same edge between two blocks only move one entry from the Phi in Old to the new Phi in New.

Diff Detail

Event Timeline

asbirlea created this revision.Aug 31 2018, 5:11 PM
george.burgess.iv accepted this revision.Sep 5 2018, 9:25 AM

Thanks!

lib/Analysis/MemorySSAUpdater.cpp
501–502

(I see that practically, the answer is "no". Just curious how strongly we're tailoring this function for the user)

Is it theoretically reasonable for Preds to contain duplicates of the same BasicBlock if !IdenticalEdgesWereMerged? The concern being that if I, as the user, call wireOldPredecessorsToNewImmediatePredecessor(Old, New, {BB, BB}, false);, we'll only end up moving one instance of BB.

If this concern is founded, please add an assert that PredsSet.size() == Preds.size() if !IdenticalEdgesWereMerged, so it's obvious that said case is broken if someone tries to depend on it in the future (which I'm totally cool with, given that, like said, this can't happen at the moment).

This revision is now accepted and ready to land.Sep 5 2018, 9:25 AM
asbirlea updated this revision to Diff 164505.Sep 7 2018, 1:44 PM
asbirlea marked an inline comment as done.

Address comment. Thank you for the review!

asbirlea added inline comments.Sep 7 2018, 1:45 PM
lib/Analysis/MemorySSAUpdater.cpp
501–502

Agreed, added the assert!

asbirlea updated this revision to Diff 164506.Sep 7 2018, 1:47 PM

Nit: remove braces.

This revision was automatically updated to reflect the committed changes.