[PATCH] D67217: [Debuginfo][Instcombiner] Do not clone dbg.declare in TryToSinkInstruction()

Wed Sep 11 07:02:06 PDT 2019

avl marked an inline comment as done.
avl added inline comments.

================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3173-3175
+        DII->setOperand(
+            0, MetadataAsValue::get(I->getContext(),
+                                    ValueAsMetadata::get(I->getOperand(0))));
----------------
aprantl wrote:
> jmorse wrote:
> > As I understand it, this works right now because the only sinkable instructions that dbg.declares point at are casts of other things (like arguments). This is probably fine -- but if dbg.declares operand rules ever change or someone writes IR that doesn't match that expectation, getOperand/setOperand could fail and it won't be clear why.
> > 
> > IMO it'd be good to add something like
> > 
> >     if (!isa<Cast>(I))
> >       continue; // dbg.declare points at something it shouldn't
> > 
> > so that the failure mode is a debug use-before-def (which I don't believe is an error) rather than a crash.
> We could probably simplify the model by enforcing that dbg.declares always point at allocas or function arguments directly, That would make code like this one look less murky.
@aprantl how should this be done in a procedural manner? I mean - Could I create a patch for verifier, which would check "that dbg.declares always point at allocas or function arguments directly" and then fix all places ? Or that question should be previously discussed somehow? 

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67217