Page MenuHomePhabricator

[MemorySSA] Avoid adding Phis in the presence of unreachable blocks.

Authored by asbirlea on Sep 24 2019, 4:05 PM.



If a block has all incoming values with the same MemoryAccess (ignoring
incoming values from unreachable blocks), then use that incoming
MemoryAccess and do not create a Phi in the first place.

Revert IDF work-around added in rL372673; it should not be required unless
the Def inserted is the first in its block.

The patch also cleans up a series of tests, added during the many
iterations on insertDef.

The patch also fixes PR43438.
The same issue that occurs in insertDef with "adding phis, hence the IDF of
Phis is needed", can also occur in fixupDefs: the getPreviousRecursive
call only adds Phis walking on the predecessor edges, which means there
may be the case of a Phi added walking the CFG "backwards" which
triggers the needs for an additional Phi in successor blocks.
Such Phis are added during fixupDefs only in the presence of unreachable
Hence this highlights the need to avoid adding Phis in blocks with
unreachable predecessors in the first place.

Diff Detail


Event Timeline

asbirlea created this revision.Sep 24 2019, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 4:05 PM


82 ↗(On Diff #221617)

nit: would it be simpler to keep a count of the unreachable blocks we saw (not adding anything to PhiOps), then if PhiOps.size() > 1, just adding NumUnreachablePreds LiveOnEntryDefs?

This revision is now accepted and ready to land.Sep 25 2019, 2:22 PM
asbirlea edited the summary of this revision. (Show Details)Sep 25 2019, 3:14 PM
asbirlea updated this revision to Diff 221846.Sep 25 2019, 3:25 PM

Add testcase.

asbirlea marked 2 inline comments as done.Sep 25 2019, 4:21 PM
asbirlea added inline comments.
82 ↗(On Diff #221617)

I think having the SingleAccess is important to not end up adding Phis when multiple reachable blocks provide the same incoming access. Given this, we can't really model everything with a single counter. Yes, we can use a counter and add the LiveOnEntryDefs after the loop, but it's roughly the same amount of code/same complexity.

This revision was automatically updated to reflect the committed changes.
asbirlea marked an inline comment as done.