Page MenuHomePhabricator

[DebugInfo] Correctly coalesce DBG_VALUEs that mix direct and indirect values
ClosedPublic

Authored by rnk on Sep 15 2017, 2:30 PM.

Details

Summary

This should fix a regression introduced by r313786, which switched from
MachineInstr::isIndirectDebugValue() to checking if operand 1 is an
immediate. I didn't have a test case for it until now.

A single UserValue, which approximates a user variable, may have many
DBG_VALUE instructions that disagree about whether the variable is in
memory or in a virtual register. This will become much more common once
we have llvm.dbg.addr, but you can construct such a test case manually
today with llvm.dbg.value.

Before this change, we would get two UserValues: one for direct and one
for indirect DBG_VALUE instructions describing the same variable. If we
build separate interval maps for direct and indirect locations, we will
end up accidentally coalescing identical DBG_VALUE intervals that need
to remain separate because they are broken up by intervals of the
opposite direct-ness.

Event Timeline

rnk created this revision.Sep 15 2017, 2:30 PM
rnk retitled this revision from [DebugInfo] Move the "indirect" bit to the location interval map to [DebugInfo] Correctly coalesce DBG_VALUEs that mix direct and indirect values.Sep 20 2017, 12:01 PM
rnk edited the summary of this revision. (Show Details)
rnk updated this revision to Diff 116048.Sep 20 2017, 12:02 PM
  • rebase
dblaikie added inline comments.
llvm/lib/CodeGen/LiveDebugVariables.cpp
119–122

Generally, binary op overloads should be non-members (they can be friends though, which can be defined inline) to ensure equal conversions are possible on both sides of the operand.

aprantl added inline comments.Sep 25 2017, 2:17 PM
llvm/lib/CodeGen/LiveDebugVariables.cpp
110

I find this confusing.. we only have 32 bits available for LocNo.. can this every fire?

aprantl added inline comments.Sep 25 2017, 2:29 PM
llvm/lib/CodeGen/LiveDebugVariables.cpp
110

s/32/31/

rnk added inline comments.Sep 25 2017, 9:55 PM
llvm/lib/CodeGen/LiveDebugVariables.cpp
110

It's INT_MAX, not UINT_MAX, i.e. 0x7FFFFFFF, so it's the all-ones value of the 31-bit bitfield. I wasn't sure what the best way to express this was. =/

rnk added a comment.Oct 2 2017, 5:34 PM

ping, I want to make sure nobody else has to debug the regression. :)

aprantl accepted this revision.Oct 2 2017, 7:36 PM
This revision is now accepted and ready to land.Oct 2 2017, 7:36 PM
This revision was automatically updated to reflect the committed changes.
danilaml added inline comments.
llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp
195 ↗(On Diff #117547)

What does that mean/imply? I've come across a case/bug that would be solved by not emitting same DBG_VALUE that differ only in that one has !DIExpression() and the other !DIExpression(DW_OP_LLVM_fragment, 0, <reg_size_in_bits>). Are those supposed to be the equivalent and one can safely emit a singe !DIExpression() instead? What caused this to remain as FIXME in the first place?

Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 4:20 AM
aprantl added inline comments.Aug 20 2019, 8:51 AM
llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp
195 ↗(On Diff #117547)

!DIExpression(DW_OP_LLVM_fragment, 0, <reg_size_in_bits>) could a special case that could just mean that sizeof(variable)-reg_size bits are unused padding, in which case it would have been better to emit a cast/sign-extend operation instead of a fragment, but that is hard to tell without the concrete example. Are there remaining upper bits in your variable that are significant? In any case, it is not generally safe to treat the fragment at offset 0 as equivalent to an empty expression, but there may be ways to void the fragment earlier in the compiler.

danilaml added inline comments.Aug 20 2019, 9:36 AM
llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp
195 ↗(On Diff #117547)

The variable is the same as reg in size as far as I can see (signed 32-bit integer).

I have something like

DBG_VALUE $r1, $noreg, !"i", !DIExpression()
$r2 = COPY $r1
DBG_VALUE $r2, $noreg, !"i", !DIExpression()
<some unrelated instr>
DBG_VALUE $r2, $noreg, !"i", !DIExpression(DW_OP_LLVM_fragment, 0, 32)
$r1 = ADD $r2, 1
DBG_VALUE $r1, $noreg, !"i", !DIExpression()
...

which is then broken by mi scheduling since LLVM only moves adjacent DBG_VALUES when it reschedules instructions (in this case it moves above copy before ADD leaving now wrong info fro $r2 after <some unrelated instr>).

This would be solved if !DIExpression() was coalesced with the fragment (so that all relevant DBG_VALUEs always follow the instruction in my case). I've noticed this FIXME this might be read as just that, hence asked to elaborate (I understand that preferable solution would be to (also) fix scheduler, but its obviously more involved).

aprantl added inline comments.Aug 20 2019, 9:44 AM
llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp
195 ↗(On Diff #117547)

Can you figure out which pass creates the fragment? (Probably SROA?)

danilaml added inline comments.Aug 20 2019, 10:14 AM
llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp
195 ↗(On Diff #117547)

It happens during DAG selection (although I don't see any debug values in its output). Last IR has no fragments ad first MIR has them. It could be the implicit result of some lowering but I don't know anything that might've caused it (possibly byproduct of some pattern match?). Anyway, how does that relate the question of whether empty empty expression on some reg is equivalent to fragment(0, size)? Or are you implying that fragment dbg value is wrong in this case and shouldn't have been generated?

aprantl added inline comments.Aug 20 2019, 11:21 AM
llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp
195 ↗(On Diff #117547)

Perhaps it's the DAG legalizer creating it?

If a fragment was created for a good reason, then an empty expression which covers the entire variable, is not equivalent to an fragment at offset 0 because there must be upper bits of the variable that are not covered by that fragment expression. But if the expression is redundant, because the fragment does cover the entire variable it perhaps shouldn't have been created, and that's the bug you'll want to fix.

danilaml added inline comments.Aug 21 2019, 3:33 AM
llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp
195 ↗(On Diff #117547)

Don't see anything suspicious with legalizer. The only thing I see in adjacent instruction is "ISEL: Starting pattern match" morphing add node into add_with_immediate (using patter from tablegen, I gather). Also, I don't quite get what you mean by "because there must be upper bits of the variable that are not covered by that fragment expression", since the variable, the register and the node type (i32) are all 32 bits. Perhaps fragments are added by tablegen pattern match to be conservative? Anyway, is it really a bug? It could be some sort of deficiency, but what is wrong with adding such fragments? Do they change the semantics of the debug info? I.e. do they differ in meaning from empty di expression? If not, then what's wrong with treating them equivalent in the function above? What does the FIXME comment mean by "The fragment should be part of the equivalence class"?

aprantl added inline comments.Aug 21 2019, 8:47 AM
llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp
195 ↗(On Diff #117547)

What does the FIXME comment mean by "The fragment should be part of the equivalence class"

Two dbg.values describing the same (inlined instance of a) variable, with no fragment expression or overlapping fragment expressions describe the same entity and therefore the second dbg.value truncates the live range of the first and changes the location of that variable (fragment). Two dbg.values with non-overlapping fragment expressions are independent from each other, as if they were describing two different variables.

You were asking whether an empty DIExpression can be treated as the same as a fragment at offset 0, and the answer is no, because the upper bits are missing. If the fragment does cover the entire variable then the code that generated the fragment should be fixed, but you can't safely merge the fragment and the non-fragment here.