This is an archive of the discontinued LLVM Phabricator instance.

[MachineDebugify] Insert synthetic DBG_VALUE instructions
ClosedPublic

Authored by vsk on Apr 14 2020, 11:05 AM.

Details

Summary

Teach MachineDebugify how to insert DBG_VALUE instructions. This can
help find bugs causing CodeGen differences when debug info is present.
DBG_VALUE instructions are only emitted when -debugify-level is set to
locations+variables.

There is essentially no attempt made to match up DBG_VALUE register
operands with the local variables they ought to correspond to. I'm not
sure how to improve the situation. In some cases (MachineMemOperand?)
it's possible to find the IR instruction a MachineInstr corresponds to,
but in general this seems to call for "undoing" the work done by ISel.

Diff Detail

Event Timeline

vsk created this revision.Apr 14 2020, 11:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2020, 11:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
vsk updated this revision to Diff 257441.Apr 14 2020, 12:09 PM

Do not insert DBG_VALUEs after phis, this is illegal. Test coverage for this comes from legalize-phi-insertpt-decrement.mir.

There is essentially no attempt made to match up DBG_VALUE register
operands with the local variables they ought to correspond to. I'm not
sure how to improve the situation.

Yeah, and it gets harder the further down the pipeline you go. Especially since the pipeline is fairly free form and even early passes are allowed to select target instructions. I don't think it really matters too much if the IR-level dbg.value maps to the 'correct' MIR-level vreg as we're really looking for places the DBG_VALUE causes crashes or causes CodeGen changes. Eventually we might expand into checking that DBG_VALUE's are handled as opposed to accidentally lost. Even then though we're more concerned with preserving MIR level information rather than the actual mapping to IR-level

llvm/lib/CodeGen/MachineDebugify.cpp
55–64

I notice that this only generates enough information to cover the line numbers in the IR-level. That limits the vregs covered by DBG_VALUE's to the first few as there's typically more MIR instructions than IR instructions. Is it possible to cover more of the MIR vregs?

llvm/test/CodeGen/Generic/MIRDebugify/locations-and-values.mir
1

We ought to rename this test if it's going to cover the DBG_VALUE case too

vsk updated this revision to Diff 257576.Apr 14 2020, 5:49 PM

Cover each non-phi reg def with a DBG_VALUE, per Daniel's suggestion.

I thought I fixed the last CodeGen-difference-with-debug bug in the AArch64 backend with D78157, but with this change, I've found a few more :).

vsk marked 2 inline comments as done.Apr 14 2020, 5:49 PM
paquette added inline comments.Apr 15 2020, 2:15 PM
llvm/lib/CodeGen/MachineDebugify.cpp
59–62

Maybe a bit nicer:

if (Line2Var.empty())
  continue;

for (MachineOperand &MO : MI.operands())
  ...
vsk marked an inline comment as done.Apr 15 2020, 2:40 PM
vsk updated this revision to Diff 257858.Apr 15 2020, 2:40 PM

Early-exit in loop

vsk updated this revision to Diff 258131.Apr 16 2020, 12:35 PM

One more (and I think the last!) update to the testing strategy.

Instead of limiting -mir-debugify to inserting DBG_VALUEs after register
definitions, have it insert DBG_VALUEs after /every/ real instruction. When a
register def isn't available, just describe a constant.

Also, sometimes MIR tests are written with skeletal (or no) IR. We still want
-mir-debugify test coverage in these cases, so force the IR debugify pass to
describe at least one variable per function.

This uncovers a few more CodeGen-difference-with-debug issues.

This revision is now accepted and ready to land.Apr 21 2020, 2:08 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.

Looks like the tests added here fail:
http://lab.llvm.org:8011/#/builders/109/builds/4730
http://45.33.8.238/linux/35299/step_12.txt

Since the tree is in pretty bad shape right now (this is at least the 3rd consecutive breakage), I'm reverting this.

Sorry, I somehow thought this landed an hour ago, not many months. Will reland, apologies :/