[DebugInfo] Factor out SalvageDebugInfoForDbgValues() from InstCombine

[chrisjackson]

Summary

The SalvageDebugInfoForDbgValues() function is exposed and used externally only in InstCombine. Refactor InstCombine to instead use SalvageDebugInfo() and keep SalvageDebugInfoForDbgValues() internal.
However, this relies on the internal behaviour of SalvageDbgInfo(), specifically the way findDbgValues() behaves when it encounters undef. This patch builds on top of D78369, which also simplifies the exposed debuginfo salvaging functions.

The guard in InstructionCombining.cpp:3477

if(!DIIClones.empty())

is used to prevent a salvage occurring in a scenario not possible in the current implementation. Without this a salvage can be executed without having cloned any Users. Doing so may not be incorrect, but it certainly would be different to the current behaviour.

[vsk]

I personally find the existing code easier to read/understand. Is there a strong motivation for hiding salvageDebugInfoForDbgValues?

[chrisjackson]

In D78398#1989878, @vsk wrote:

I personally find the existing code easier to read/understand. Is there a strong motivation for hiding salvageDebugInfoForDbgValues?

The intention was to reduce the variety of salvage calls that are made, making salvaging easier to understand and reducing errors by reducing the choice of calls. In the original InstCombine code we make repeated salvage calls against the instruction instead of just one, which is unusual.

[Orlando]

Assuming D78369 lands as it is right now; IMO it might make sense to push the OrUndef behaviour of salvageDebugInfo into salvageDebugInfoForDbgValues, and have salvageDebugInfo simply call salvageDebugInfoForDbgValues(I, findDbgUsers(I)).

This is pretty similar to what happens pre-D78369, except that salvageDebugInfo and salvageDebugInfoForDbgValues both MarkUndef if they fail to salvage. I'm fairly confident you could then simplify the instcombine sink salvaging with this setup.

What do you both think?

[djtodoro]

If this relies on D78369, please make a stack of these patches.

llvm/lib/Transforms/Utils/Local.cpp

1649

These functions need comments describing the intention.

[vsk]

In D78398#1992348, @chrisjackson wrote:

In D78398#1989878, @vsk wrote:

I personally find the existing code easier to read/understand. Is there a strong motivation for hiding salvageDebugInfoForDbgValues?

The intention was to reduce the variety of salvage calls that are made, making salvaging easier to understand and reducing errors by reducing the choice of calls. In the original InstCombine code we make repeated salvage calls against the instruction instead of just one, which is unusual.

That's fair, that's a good goal. I admit the formatting of the patch really made it harder to get a sense for what it's doing -- could you please clang-format the diff?

In D78398#1992390, @Orlando wrote:

Assuming D78369 lands as it is right now; IMO it might make sense to push the OrUndef behaviour of salvageDebugInfo into salvageDebugInfoForDbgValues, and have salvageDebugInfo simply call salvageDebugInfoForDbgValues(I, findDbgUsers(I)).

This is pretty similar to what happens pre-D78369, except that salvageDebugInfo and salvageDebugInfoForDbgValues both MarkUndef if they fail to salvage. I'm fairly confident you could then simplify the instcombine sink salvaging with this setup.

What do you both think?

Makes sense to me.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3463

@jmorse Is the reverse really needed? Not sure the iteration order in findDbgUsers has any meaning.

3465

Please use early-exits where you can to reduce nesting.

[chrisjackson]

Applied clang-format-diff.

[jmorse]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3463

Looks like I mentioned that on the last line of this comment: https://reviews.llvm.org/D56788#1394898

It's not something that inspires confidence; I think in retrospect that additional salvaging meant additional sinking, giving the test mentioned in that comment an extra dbg.value that isn't tested for. There are three dbg.values now, and only two are tested for. At the time, I probably mistook a new / extra dbg.value as an accidental change in ordering.

I agree the order in findDbgUsers isn't meaningful, the reverse can go.

[chrisjackson]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3463

I considered removing the reverse but doing so changed the ordering of some output and required a change in test/Transforms/InstCombine/debuginfo_add.ll. I will update the code and test.

[chrisjackson]

• Removed the call to reverse() in InstCombine.cpp and updated the debuginfo_add.ll test accordingly.
• Moved some comments in InstCombine to a more appropriate spot.
• Simplified an iteration in InstCombine as suggested.

[vsk]

I think this is in good shape.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3442

nit, 'arguments of' -> 'dbg.declare arguments'?

3445

nit, (if I'm reading this correctly) it should be possible to remove a level of nesting by doing both isa checks in the first if

3465

nit: !=

[chrisjackson]

In D78398#1992390, @Orlando wrote:

Assuming D78369 lands as it is right now; IMO it might make sense to push the OrUndef behaviour of salvageDebugInfo into salvageDebugInfoForDbgValues, and have salvageDebugInfo simply call salvageDebugInfoForDbgValues(I, findDbgUsers(I)).

This is pretty similar to what happens pre-D78369, except that salvageDebugInfo and salvageDebugInfoForDbgValues both MarkUndef if they fail to salvage. I'm fairly confident you could then simplify the instcombine sink salvaging with this setup.

What do you both think?

This seems reasonable. I'll complete the requested changes to this diff then I'll submit one with @Orlando's suggested approach and we can see which is looks better.

In D78398#1992390, @Orlando wrote:

Assuming D78369 lands as it is right now; IMO it might make sense to push the OrUndef behaviour of salvageDebugInfo into salvageDebugInfoForDbgValues, and have salvageDebugInfo simply call salvageDebugInfoForDbgValues(I, findDbgUsers(I)).

This is pretty similar to what happens pre-D78369, except that salvageDebugInfo and salvageDebugInfoForDbgValues both MarkUndef if they fail to salvage. I'm fairly confident you could then simplify the instcombine sink salvaging with this setup.

What do you both think?

I have uploaded a diff implementing this suggested alternative, D79863.

[chrisjackson]

Abandoned in favour of the alternative D79863.

llvm/include/llvm/Transforms/Utils/Local.h

                                DenseMap<Value *, AllocaInst *> &AllocaForValue);
 
 /// Assuming the instruction \p I is going to be deleted, attempt to salvage
 /// debug users of \p I by writing the effect of \p I in a DIExpression. If it
 /// cannot be salvaged mark it undef.
 /// Returns true if any debug users were updated.
 bool salvageDebugInfo(Instruction &I);
 
-
-/// Implementation of salvageDebugInfo, applying only to instructions in
-/// \p Insns, rather than all debug users of \p I.
-bool salvageDebugInfoForDbgValues(Instruction &I,
-                                  ArrayRef<DbgVariableIntrinsic *> Insns);
-
 /// Given an instruction \p I and DIExpression \p DIExpr operating on it, write
 /// the effects of \p I into the returned DIExpression, or return nullptr if
 /// it cannot be salvaged. \p StackVal: whether DW_OP_stack_value should be
 /// appended to the expression.
 DIExpression *salvageDebugInfoImpl(Instruction &I, DIExpression *DIExpr,
                                    bool StackVal);
 
 /// Point debug users of \p From to \p To or salvage them. Use this function

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

 
   // Also sink all related debug uses from the source basic block. Otherwise we
   // get debug use before the def. Attempt to salvage debug uses first, to
   // maximise the range variables have location for. If we cannot salvage, then
   // mark the location undef: we know it was supposed to receive a new location
   // here, but that computation has been sunk.
   SmallVector<DbgVariableIntrinsic *, 2> DbgUsers;
   findDbgUsers(DbgUsers, I);
-  for (auto *DII : reverse(DbgUsers)) {
-    if (DII->getParent() == SrcBlock) {
-      if (isa<DbgDeclareInst>(DII)) {
-        // A dbg.declare instruction should not be cloned, since there can only be
-        // one per variable fragment. It should be left in the original place since
-        // sunk instruction is not an alloca(otherwise we could not be here).
-        // But we need to update arguments of dbg.declare instruction, so that it
-        // would not point into sunk instruction.
-        if (!isa<CastInst>(I))
-          continue; // dbg.declare points at something it shouldn't
 
+  // We update arguments of so that the instruction does not point into sunk
+  // instruction.
+  auto updateDbgDeclare = [&I](DbgVariableIntrinsic *DII) {
+    if (isa<DbgDeclareInst>(DII)) {
+      if (isa<CastInst>(I))
         DII->setOperand(
             0, MetadataAsValue::get(I->getContext(),
                                     ValueAsMetadata::get(I->getOperand(0))));
-        continue;
-      }
 
-      // 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);
-      }
+      return true;
+    }
+    return false;
+  };
+
+  // We do not want salvageDebugInfo() to modify the clones. Prevent
+  // modification by setting each clones Op0 to Undef. The original value
+  // is restored when the clone is sunk.
+  SmallVector<std::pair<DbgVariableIntrinsic *, llvm::Value *>, 2> DIIClones;
+  for (auto &User : DbgUsers) {
+    // A dbg.declare instruction should not be cloned, since there can only be
+    // one per variable fragment. It should be left in the original place
+    // because the sunk instruction is not an alloca (otherwise we could not be
+    // here).
+    if (!(User->getParent() == SrcBlock) || updateDbgDeclare(User))
+      continue;
+
+    DbgVariableIntrinsic *clone = cast<DbgVariableIntrinsic>(User->clone());
+    DIIClones.emplace_back(std::make_pair(clone, clone->getOperand(0)));
+    LLVM_DEBUG(dbgs() << "CLONE: " << DIIClones.back().first << '\n');
+    Value *Undef = UndefValue::get(I->getType());
+    clone->setOperand(0, MetadataAsValue::get(clone->getContext(),
+                                              ValueAsMetadata::get(Undef)));
+  }
+
+  if (!DIIClones.empty()) {
+    salvageDebugInfo(*I);
+    for (auto &DIIClone : DIIClones) {
+      DIIClone.first->setOperand(0, DIIClone.second);
+      DIIClone.first->insertBefore(&*InsertPos);
+      LLVM_DEBUG(dbgs() << "SINK: " << *DIIClone.first << '\n');
     }
   }
+
   return true;
 }
 
 bool InstCombiner::run() {
   while (!Worklist.isEmpty()) {
     // Walk deferred instructions in reverse order, and push them to the
     // worklist, which means they'll end up popped from the worklist in-order.
     while (Instruction *I = Worklist.popDeferred()) {

llvm/lib/Transforms/Utils/Local.cpp

       }
 }
 
 /// Wrap \p V in a ValueAsMetadata instance.
 static MetadataAsValue *wrapValueInMetadata(LLVMContext &C, Value *V) {
   return MetadataAsValue::get(C, ValueAsMetadata::get(V));
 }
 
-static bool attemptToSalvageDebugInfo(Instruction &I) {
-  SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
-  findDbgUsers(DbgUsers, &I);
-  if (DbgUsers.empty())
-    return false;
-
-  return salvageDebugInfoForDbgValues(I, DbgUsers);
-}
-
-bool llvm::salvageDebugInfo(Instruction &I) {
-  bool salvaged = attemptToSalvageDebugInfo(I);
-  if (!salvaged)
-    replaceDbgUsesWithUndef(&I);
-  return salvaged;
-}
-
-bool llvm::salvageDebugInfoForDbgValues(
-    Instruction &I, ArrayRef<DbgVariableIntrinsic *> DbgUsers) {
+static bool
+salvageDebugInfoForDbgValues(Instruction &I,
+                             ArrayRef<DbgVariableIntrinsic *> DbgUsers) {
   auto &Ctx = I.getContext();
   auto wrapMD = [&](Value *V) { return wrapValueInMetadata(Ctx, V); };
 
   for (auto *DII : DbgUsers) {
     // Do not add DW_OP_stack_value for DbgDeclare and DbgAddr, because they
     // are implicitly pointing out the value as a DWARF memory location
     // description.
     bool StackValue = isa<DbgValueInst>(DII);
 
     DIExpression *DIExpr =
         salvageDebugInfoImpl(I, DII->getExpression(), StackValue);
 
     // salvageDebugInfoImpl should fail on examining the first element of
     // DbgUsers, or none of them.
     if (!DIExpr)
       return false;
 
     DII->setOperand(0, wrapMD(I.getOperand(0)));
     DII->setOperand(2, MetadataAsValue::get(Ctx, DIExpr));
     LLVM_DEBUG(dbgs() << "SALVAGE: " << *DII << '\n');
   }
 
   return true;
 }
 
+/// If the instruction \p I has any debug users salvage the debug
+/// information and return True. Else return False.
+static bool attemptToSalvageDebugInfo(Instruction &I) {
+  SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
+  findDbgUsers(DbgUsers, &I);
+  if (DbgUsers.empty())
+    return false;
+
+  return salvageDebugInfoForDbgValues(I, DbgUsers);
+}
+
+/// Where possible to salvage debug information for \p I do so
+/// and return True. If not possible mark uses of I as undef and
+/// return False.
+bool llvm::salvageDebugInfo(Instruction &I) {
+  bool salvaged = attemptToSalvageDebugInfo(I);
+  if (!salvaged)
+    replaceDbgUsesWithUndef(&I);
+  return salvaged;
+}
+
 DIExpression *llvm::salvageDebugInfoImpl(Instruction &I,
                                          DIExpression *SrcDIExpr,
                                          bool WithStackValue) {
   auto &M = *I.getModule();
   auto &DL = M.getDataLayout();
 
   // Apply a vector of opcodes to the source DIExpression.
   auto doSalvage = [&](SmallVectorImpl<uint64_t> &Ops) -> DIExpression * {

llvm/test/Transforms/InstCombine/debuginfo_add.ll

entry:
   tail call void @llvm.dbg.value(metadata i32 undef, metadata !23, metadata !DIExpression()), !dbg !32
   br i1 undef, label %for.end, label %for.body.lr.ph, !dbg !32
 
 for.body.lr.ph:                                   ; preds = %entry
   ; The 'load' and the 'add' are sunken to this basic block. So let's verify that the related dbg.values are sunken as well.
   ; The add is later eliminated, so we verify that the dbg.value is salvaged by using DW_OP_minus.
   ; CHECK-LABEL: for.body.lr.ph:
   ; CHECK-NEXT: %0 = load
-  ; CHECK-NEXT: call void @llvm.dbg.value(metadata i64 %0, metadata !25, metadata !DIExpression()), !dbg !
   ; 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
 
 for.body:                                         ; preds = %for.body.lr.ph, %for.body
   ; CHECK-LABEL: for.body:
   %offset.010 = phi i64 [ %offset.08, %for.body.lr.ph ], [ %offset.0, %for.body ]
   %head_size.09 = phi i32 [ undef, %for.body.lr.ph ], [ %sub2, %for.body ]
   tail call void @llvm.dbg.value(metadata i32 %head_size.09, metadata !23, metadata !DIExpression()), !dbg !31
   %call = tail call i32 bitcast (i32 (...)* @use to i32 (i64, %struct.vm_object*)*)(i64 %offset.010, %struct.vm_object* %object) #3, !dbg !34