Page MenuHomePhabricator

Ensure that we don't update Uses after MarkLive
Needs ReviewPublic

Authored by Aaron1011 on Sep 10 2020, 11:35 AM.
This revision needs review, but all reviewers have resigned.


Fixes bug #47444

Previously, we would call MarkValue for each RetOrArg, either calling
MarkLive or inserting dependencies into Uses. However, this can cause us
to end up calling MarkLive for a RetOrArg, and then later inserting
additional dependencies into Uses. As a result, the liveness propagation
during MarkLive will miss some dependencies, causing us to incorrectly
see a RetOrArg as dead.

I've fixed this by replacing MarkValue with a new function MarkValues,
which takes in a vector of RetOrArgs to inspect. It works in two passes
- we first insert all dependencies into Uses, and then call MarkLive for
any live values. This ensures that liveness is correctly propagated to
all dependencies.

To ensure that this type of issue doesn't occur again, I've added an
assertion to verify that we don't try to insert dependencies into Uses
for a value that has been already marked live.

I've included a minimized version of the IR from bug #47444 as a test
case. Without this pass, return value #1 is incorrectly removed from
the function 'g'.

Diff Detail

Event Timeline

Aaron1011 created this revision.Sep 10 2020, 11:35 AM
Aaron1011 requested review of this revision.Sep 10 2020, 11:35 AM

The pre-merge check seems to want markValues instead of MarkValues, even though the method is curretly 'MarkValue'. Should I change this?

nikic added a subscriber: nikic.Sep 22 2020, 2:29 PM
rnk edited reviewers, added: aeubanks; removed: rnk.Sep 23 2020, 11:05 AM
rnk added a subscriber: rnk.

Arthur, do you mind taking a look?

Yep I'll take a look. seems like a nicer solution, wdyt?

aeubanks resigned from this revision.Dec 22 2020, 12:37 PM