Page MenuHomePhabricator

[DAGCombine] Improve Lifetime node chains.

Authored by niravd on Mar 25 2019, 12:31 PM.



Improve both start and end lifetime nodes chain dependencies.

Diff Detail


Event Timeline

niravd created this revision.Mar 25 2019, 12:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2019, 12:31 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Nice !

15575 ↗(On Diff #192184)

Please refactor with the code in visitLIFETIME_END

15585 ↗(On Diff #192184)

I have a hard time convincing myself that we can omit this check. Can you add a comment to the code to explain why ?

15594 ↗(On Diff #192184)

please clang-format the patch.

niravd updated this revision to Diff 192343.Mar 26 2019, 1:37 PM

Extract copied code into helper and format

niravd marked 3 inline comments as done.Mar 26 2019, 7:11 PM
niravd added inline comments.
15585 ↗(On Diff #192184)

It's not strictly; we could accidentally skip a store because the TokenFactors aren't properly minimized. There needs to be an extra check that the found store is the last semantically meaningful before the LIFETIME_END. I'll factor this out separately from the chain improvement logic.

courbet added inline comments.Mar 27 2019, 5:01 AM
1600 ↗(On Diff #192343)

Maybe avoid clang-formatting this for consistency with the rest of the function ? (sigh...)

15585 ↗(On Diff #192184)

we could accidentally skip a store because the TokenFactors aren't properly minimized.

What happens then ? :)

Can you add this comment to the code so that future readers are aware of the choices made here ?

niravd updated this revision to Diff 192682.Mar 28 2019, 10:55 AM

Now that lifetime node hashing and TokenFactor cleanup has happened, factor out Dead Store improvements as they are no longer needed to prevent regressions.

courbet accepted this revision.Mar 29 2019, 1:40 AM

Thanks, just one nit.

15670 ↗(On Diff #192682)

This can be simplified to:

return ImproveLifetimeNodeChain(N);
This revision is now accepted and ready to land.Mar 29 2019, 1:40 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Apr 3 2019, 12:45 AM

This was reverted together with r357309 in r357563 as it caused