[PATCH] D37932: [DebugInfo] Correctly coalesce DBG_VALUEs that mix direct and indirect values

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp:195
+    // FIXME: The fragment should be part of the equivalence class, but not
+    // other things in the expression like stack values.
+    return Var == Variable && Expr == Expression && dl->getInlinedAt() == IA;
----------------
danilaml wrote:
> aprantl wrote:
> > danilaml wrote:
> > > aprantl wrote:
> > > > danilaml wrote:
> > > > > aprantl wrote:
> > > > > > danilaml wrote:
> > > > > > > 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?
> > > > > > `!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.
> > > > > 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).
> > > > Can you figure out which pass creates the fragment? (Probably SROA?)
> > > 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?
> > 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.
> 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"?
> 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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D37932/new/

https://reviews.llvm.org/D37932