[PATCH] D56788: [DebugInfo][InstCombine] Prefer salvaging dbg.values over sinking them

Thu Feb 14 03:51:39 PST 2019

bjope added inline comments.

================
Comment at: llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp:3122
+        Value *Undef = UndefValue::get(I->getType());
+        DII->setOperand(0, MetadataAsValue::get(DII->getContext(),
+                                                ValueAsMetadata::get(Undef)));
----------------
jmorse wrote:
> bjope wrote:
> > (post commit comment)
> > I see things like this in the IR after this:
> > 
> >   call void @llvm.dbg.value(metadata %foo * undef, metadata !2292, metadata !DIExpression(DW_OP_plus_uconst, 3, DW_OP_stack_value)), !dbg !2760
> > 
> > It doesn't fill any purpose to have a complicated DIExpression when being based on an undef value afaict.
> > Maybe we should strip away the DIExpression right away here, or what do you think? 
> I agree, it's pointless memory use and a distraction (or even misleading). This should probably be rolled into a DbgVariableIntrinsic method that sets operand-0 to undef *and* clears the expression.
> 
> I imagine that for fragments of larger variables though, we would need to keep the DW_OP_LLVM_fragment so that only that portion of the variable gets undef'd.
> 
> (I can't follow this up with code for about a week due to other backlogs alas).
Yes, good catch. We still need it (or at least parts of the DIExpression) for fragments, I did not think about that.

Anyway, this won't cost anything in the final DWARF location lists, so it is not that important. It is just a little bit confusing when looking at the IR, and it might have a negligible cost in the IR.

(Maybe I'll find some time to fix it. Although not on top of the priority list right now. And I do not think it is worth writing a PR for this.)

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56788