Page MenuHomePhabricator

[MemorySSA] Rename uses when inserting memory uses.

Authored by asbirlea on Aug 9 2019, 3:05 PM.



When inserting uses from outside the MemorySSA creation, we don't
normally need to rename uses, based on the assumption that there will be
no inserted Phis (if Def existed that required a Phi, that Phi already
exists). However, when dealing with unreachable blocks, MemorySSA will
optimize away Phis whose incoming blocks are unreachable, and these Phis end
up being re-added when inserting a Use.
There are two potential solutions here:

  1. Analyze the inserted Phis and clean them up if they are unneeded

(current method for cleaning up trivial phis does not cover this)

  1. Leave the Phi in place and rename uses, the same way as whe inserting

This patch use approach 2.

Resolves first test in PR42940.

Diff Detail


Event Timeline

asbirlea created this revision.Aug 9 2019, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 3:05 PM

Note: this is incomplete.

We can always have Phis with LoE coming from unreachable blocks, but when doing an insertDef, we replace all uses of the previous def with self. If that def is LoE, we'll end up replacing the incoming value from those unreachable blocks too. So the verification of "if incoming from unreachable, then it must be LoE is too strict".

With this restriction lifted, there is still a failure from the original PR. I will update once I have the fix for that.

asbirlea updated this revision to Diff 214866.Aug 13 2019, 10:12 AM

Updated move call to also rename Uses.
Removed asserts checking against LoE.
Added two more reduced tests.

asbirlea updated this revision to Diff 215469.Aug 15 2019, 2:22 PM

Rebase on ToT.

lgtm with a few nits. Thanks!

233 ↗(On Diff #214866)

nit: can we please add a bit to the comment above instead of immediately contradicting it? e.g.

"In cases without unreachable blocks, because uses do not create new may-defs, there are only two cases: [...]"

"In cases with unreachable blocks, where the unnecessary Phis were optimized out, adding the Use [...]"

237 ↗(On Diff #214866)

Is there a cheap way to assert that no renaming needs to be done if RenameUses == false?

Even if it might fail to catch some cases, if we can do something as simple as if (!RenameUses && !InsertedPHIs.empty()) assert(TheBBContainingMUHasNoDefsExceptForThisPhi); that can catch trivial misuses might be valuable.

253 ↗(On Diff #214866)

s/dyn_cast/cast/ please

also please sink into the if condition

1103 ↗(On Diff #214866)

nit: /*RenameUses=*/

This revision is now accepted and ready to land.Aug 15 2019, 5:18 PM
asbirlea updated this revision to Diff 215936.Aug 19 2019, 9:53 AM
asbirlea marked 4 inline comments as done.

Address comments.

asbirlea updated this revision to Diff 215959.Aug 19 2019, 11:32 AM

Remove verification assert when incoming value from unreachable can be any access.
Add additional test which showcases the above.

This revision was automatically updated to reflect the committed changes.