Page MenuHomePhabricator

[DebugInfo][MachineCSE] Don't try to copy-propagate debuginfo for every COPY seen
ClosedPublic

Authored by jmorse on Feb 20 2019, 7:30 AM.

Details

Reviewers
aprantl
bjope
vsk
Summary

This patch alters the debug-info salvaging behaviour of MachineCSE, and adds a test for its behaviour (only one opaque test is currently sensitive to MachineCSEs DI salvaging).

In the loop in MachineCSE::PerformTrivialCopyPropagation (see patch), for an arbitrary input instruction, MachineCSE tries to copy-propagate vreg reading operands to expose more CSE opportunities. This is great -- but the current call to changeDebugValuesDefReg then unconditionally rewrites the DBG_VALUE users of the COPY that was propagated. This means that any COPY instruction whose result is the subject of CSE will have its DBG_VALUEs copy-propagated too, even if the COPY lives on, which is a bit overkill. I believe this propagation is only necessary if the COPY is to be deleted -- hence in this patch, the DBG_VALUE updating is moved to only occur if the COPY is deleted.

While we're here, alter changeDebugValuesDefReg to find DBG_VALUEs by enumerating vreg users, which catches DBG_VALUEs that don't immediately follow the COPY. MachineCSE is the only remaining user of this function (assuming D56265 makes it in).

I've added a test that stimulates both behaviours -- first where a COPY is deleted (and thus all DBG_VALUE users must update) and one where it doesn't.

A clang-3.4 build with this patch ticks up a fractional amount, and the covered scope bytes ticks down a fraction too. The ticking down is likely because vreg liveness interacting with DBG_VALUEs is still an open problem.

Diff Detail

Event Timeline

jmorse created this revision.Feb 20 2019, 7:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 7:30 AM
aprantl added inline comments.Feb 21 2019, 8:23 AM
lib/CodeGen/MachineCSE.cpp
197

Can you leave an explanatory comment here that will prevent somebody coming in later and moving it back to old location?

vsk added a comment.Jun 28 2019, 1:20 PM

Looks reasonable to me.

lib/CodeGen/MachineInstr.cpp
2127

Reuse 'DefReg = getOperand(0).getReg()'?

jmorse updated this revision to Diff 207353.Jul 1 2019, 9:27 AM

Rebase, update jump-insts in MIR test to latest version, address review comments (cheers).

jmorse marked 2 inline comments as done.Jul 1 2019, 9:27 AM
vsk accepted this revision.Jul 10 2019, 2:37 PM

Lgtm, thanks.

This revision is now accepted and ready to land.Jul 10 2019, 2:37 PM
jmorse closed this revision.Sep 2 2019, 5:35 AM

Committed in https://reviews.llvm.org/rL370648

During which I failed to flush the buffer where I'd written a commit message and given the correct phabricator review link -- my bad :(.