[PATCH] D79863: [DebugInfo] Refactor SalvageDebugInfo and SalvageDebugInfoForDbgValues

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Hi,

I left some nits inline but otherwise I think this is good.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3332
 
-      // dbg.value is in the same basic block as the sunk inst, see if we can
-      // salvage it. Clone a new copy of the instruction: on success we need
-      // both salvaged and unsalvaged copies.
-      SmallVector<DbgVariableIntrinsic *, 1> TmpUser{
-          cast<DbgVariableIntrinsic>(DII->clone())};
-
-      if (!salvageDebugInfoForDbgValues(*I, TmpUser)) {
-        // We are unable to salvage: sink the cloned dbg.value, and mark the
-        // original as undef, terminating any earlier variable location.
-        LLVM_DEBUG(dbgs() << "SINK: " << *DII << '\n');
-        TmpUser[0]->insertBefore(&*InsertPos);
-        Value *Undef = UndefValue::get(I->getType());
-        DII->setOperand(0, MetadataAsValue::get(DII->getContext(),
-                                                ValueAsMetadata::get(Undef)));
-      } else {
-        // We successfully salvaged: place the salvaged dbg.value in the
-        // original location, and move the unmodified dbg.value to sink with
-        // the sunk inst.
-        TmpUser[0]->insertBefore(DII);
-        DII->moveBefore(&*InsertPos);
-      }
+  // we need to update arguments of a dbg.declare instruction, so that it
+  // will not point into a sunk instruction.
----------------
we -> We


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3355
+    DIIClones.emplace_back(cast<DbgVariableIntrinsic>(User->clone()));
+    LLVM_DEBUG(dbgs() << "CLONE: " << DIIClones.back() << '\n');
+  }
----------------
Is this just printing the address of the clones? Shouldn't it be `<< *DIClones.back()`?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1629
   auto &Ctx = I.getContext();
+  bool salvaged = false;
   auto wrapMD = [&](Value *V) { return wrapValueInMetadata(Ctx, V); };
----------------
While you're here you could updated salvaged -> Salvaged to match llvm style.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1659
+  }
+  return salvaged;
 }
----------------
nit: I think this would be clearer, but not a strong opinion at all.
```
if (salvaged)
   return true;

// <undef-code>
return false;
```


================
Comment at: llvm/test/Transforms/InstCombine/debuginfo_add.ll:40
   ; CHECK-NEXT: call void @llvm.dbg.value(metadata i64 %0, metadata !26, metadata !DIExpression(DW_OP_constu, 4096, DW_OP_minus, DW_OP_stack_value)), !dbg !
+  ; CHECK-NEXT: call void @llvm.dbg.value(metadata i64 %0, metadata !25, metadata !DIExpression()), !dbg !
   br label %for.body, !dbg !32
----------------
Am I right in thinking that this move is just a result of no longer calling `reverse(...)` in the debuginfo sink update code in instcombine?


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

https://reviews.llvm.org/D79863