This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Insert DW_OP_deref when spilling indirect DBG_VALUEs
ClosedPublic

Authored by rnk on Sep 15 2017, 11:08 AM.

Details

Summary

This comes up in optimized debug info for C++ programs that pass and
return objects indirectly by address. In these programs,
llvm.dbg.declare survives optimization, which causes us to emit indirect
DBG_VALUE instructions. The fast register allocator knows to insert
DW_OP_deref when spilling indirect DBG_VALUE instructions, but the
LiveDebugVariables did not until this change.

This fixes part of PR34513. I need to look into why this doesn't work at
-O0 and I'll send follow up patches to handle that.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Sep 15 2017, 11:08 AM
aprantl added inline comments.Sep 15 2017, 1:23 PM
llvm/lib/CodeGen/LiveDebugVariables.cpp
112 ↗(On Diff #115428)

Can you change the Variable type, too, for symmetry?

500 ↗(On Diff #115428)

Why can't this be API?

1058 ↗(On Diff #115428)

There's a much easier to read BuildMI variant that takes an IsIndirect flag.

1071 ↗(On Diff #115428)

shall we hide the comparison against ~0U in a self-duocumenting helper function isCondition(LocNo)?

rnk marked an inline comment as done.Sep 15 2017, 2:04 PM
rnk added inline comments.
llvm/lib/CodeGen/LiveDebugVariables.cpp
500 ↗(On Diff #115428)

I want to consider frame indices and constants as "indirect" so that they use the same UserValue. Some tests (I'll check how many) rely on this for eliminating adjacent DBG_VALUE instructions describing the same variable, like this:

DBG_VALUE %vreg3, 0, !"v" # Goes away after rewriting if we use isImm() directly
DBG_VALUE 42, 0, !"v"
mov $42, %eax
retq

In the long run, IsIndirect should not be considered when getting or creating a UserValue. I have a follow-on patch that implements this, but I split it out for incrementality.

1058 ↗(On Diff #115428)

That one only takes a register argument, though. At this point we have a MachineOperand. I tried this first, but I couldn't set the debug-use flag:

BuildMI(*MBB, I, getDebugLoc(), TII.get(TargetOpcode::DBG_VALUE))
    .add(Loc)
    .add(NewIndirect ? MachineOperand::CreateImm(0U)
                     : MachineOperand::CreateReg(0U))
    .addMetadata(Variable)
    .addMetadata(Expr);
1071 ↗(On Diff #115428)

I actually did that in the follow-up. Do you think it's worth it now?

aprantl added inline comments.Sep 15 2017, 2:25 PM
llvm/lib/CodeGen/LiveDebugVariables.cpp
500 ↗(On Diff #115428)

Can you post you work-in-progress for the follow-up? I'd like to better understand where this is heading.

1058 ↗(On Diff #115428)

Still seems silly. I don't like that the implementation detail that "direct values are encoded with a second register operand" is leaking all over the place. Should we add a BuildMI variant that is useful in this situation? There's a bunch of other places that use the same pattern and I don't like how hard these are to find.

In any case we can do this later, too.

rnk added inline comments.Sep 15 2017, 2:34 PM
llvm/lib/CodeGen/LiveDebugVariables.cpp
500 ↗(On Diff #115428)

D37932. It needs tests, though. I haven't found a way to write any without waiting for llvm.dbg.addr or manually editting MIR, so I held off on it. But, the basic idea is that a DBG_VALUE location is more than just one MachineOperand: it's that plus the IsIndirect bit.

1058 ↗(On Diff #115428)

There's a lot we can do to clean up DBG_VALUE construction and spilling. I'd like to handle these cases, get some tests in, and then come back and revisit the representation and API to build DBG_VALUEs. One idea is to have a separate DBG_ADDR instruction mirroring the llvm.dbg.addr intrinsic.

rnk updated this revision to Diff 115499.Sep 15 2017, 2:35 PM
  • Mostly s/MDNode/DILocalVariable/
aprantl accepted this revision.Sep 15 2017, 2:39 PM

Thanks! Are you running the debuginfo-tests on your changes prior to committing?

llvm/lib/CodeGen/LiveDebugVariables.cpp
500 ↗(On Diff #115428)

Thanks! It would be great if you could split out the NFC bits of the follow-up into a separate commit (hereby LGTM'ed) following shortly after this one.

This revision is now accepted and ready to land.Sep 15 2017, 2:39 PM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Sep 15 2017, 3:12 PM

Thanks! Are you running the debuginfo-tests on your changes prior to committing?

Not as a rule, since they don't run on Windows where I'm developing, but in this case, yes I did.

llvm/lib/CodeGen/LiveDebugVariables.cpp
500 ↗(On Diff #115428)

Hopefully rL313405 is what you were thinking of