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

Thu Sep 5 07:59:52 PDT 2019

jmorse added inline comments.

================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3169
+      if (isa<DbgDeclareInst>(DII)) {
+        // dbg.declare instruction could not be cloned. It should be left at
+        // original place since sunk instruction is not an alloca(otherwise we
----------------
"should" rather than "could"? Also, IMO "be left at original" should be "be left in the original"

================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3173-3175
+        DII->setOperand(
+            0, MetadataAsValue::get(I->getContext(),
+                                    ValueAsMetadata::get(I->getOperand(0))));
----------------
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.

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

https://reviews.llvm.org/D67217