[DebugInfo][CGP] Update dbg.values when updating memory address computations

[jmorse]

Summary

One of CodeGenPrepare's optimisations is to duplicate address calculations into basic blocks, so that as much information as possible can be folded into memory addressing operands. This is great -- but the dbg.value variable location intrinsics are not updated in the same way. This can lead to dbg.values referring to address computations in other blocks that will never be encoded into the DAG, while duplicate address computations are performed locally that could be used by the dbg.value. Some of these (such as non-constant-offset GEPs) can't be salvaged past.

Fix this by, whenever we duplicate an address computation into a block, looking for dbg.value users of the original memory address in the same block, and redirecting those to the local computation.

[bjope]

lib/CodeGen/CodeGenPrepare.cpp

4938 ↗

(On Diff #187432)

What happens if there is an earlier dbg-use of Repl (before MemoryInst) in this BB. Is there a risk that we introduce a dbguse-before-def scenario here? Maybe we need to check if MemoryInst dominates User here? Or is such input IR unlikely in reality?

There seems to be lots of cases above (for example reusing an earlier computation for SunkAddr), so maybe the important check is that SunkAddr dominates User. But as you understand from the question marks above, I haven't figured out if that always is guaranteed here.

Notice that I do not think about scenarios when the input IR already is "broken" (dgb-use before def) here. Just some worrying that we might introduce dbg-use-before-def here.

Actually the test case that you added has the dbg.value before the load in the BB that we sink the address computation into (the scenario I'm talking about). Isn't the address computation added just before the load? Is perhaps the test case working due to placeDbgValues moving the dbg.value?

[jmorse]

It turns out the validity of this change relies on placeDbgValues reordering (curses), a small amount of extra juggling will be required.

lib/CodeGen/CodeGenPrepare.cpp

4938 ↗

(On Diff #187432)

Actually the test case that you added has the dbg.value before the load in the BB that we sink the address computation into (the scenario I'm talking about). Isn't the address computation added just before the load? Is perhaps the test case working due to placeDbgValues moving the dbg.value?

Aha, you've got it in one there, I was inadvertently relying on placeDbgValues. I'd placed the dbg.value at the top of the block and assumed optimizeMemoryInst did-the-right-thing because the duplicated memory insts appeared above the dbg.value. (Should be easily fixable).

[jmorse]

Avoid relying on placeDbgValues for this change. We already walk (forwards) through all instructions in a block looking to optimise them, add a dbg.value visitor that rewrites the dbg.value operand if it refers to a sunk address computation.

This works because CGP already keeps a cache of sunk addresses for each block, and we walk forwards through the block (which optimizeMemoryInst relies on anyway). Thus, we will only rewrite dbg.value operands for instructions after the sunk memory computation has been produced.

The test case will need to be updated when placeDbgValues gets limited: the first dbg.value will no longer be hoisted up a block, and the second will no longer go up one inst.

llvm/lib/CodeGen/CodeGenPrepare.cpp

private:
     bool optimizeExtUses(Instruction *I);
     bool optimizeLoadExt(LoadInst *Load);
     bool optimizeShiftInst(BinaryOperator *BO);
     bool optimizeSelectInst(SelectInst *SI);
     bool optimizeShuffleVectorInst(ShuffleVectorInst *SVI);
     bool optimizeSwitchInst(SwitchInst *SI);
     bool optimizeExtractElementInst(Instruction *Inst);
     bool dupRetToEnableTailCallOpts(BasicBlock *BB, bool &ModifiedDT);
+    bool fixupDbgValue(Instruction *I);
     bool placeDbgValues(Function &F);
     bool canFormExtLd(const SmallVectorImpl<Instruction *> &MovedExts,
                       LoadInst *&LI, Instruction *&Inst, bool HasPromoted);
     bool tryToPromoteExts(TypePromotionTransaction &TPT,
                           const SmallVectorImpl<Instruction *> &Exts,
                           SmallVectorImpl<Instruction *> &ProfitablyMovedExts,
                           unsigned CreatedInstsCost = 0);
     bool mergeSExts(Function &F);
case Intrinsic::strip_invariant_group: {
       II->replaceAllUsesWith(ArgVal);
       II->eraseFromParent();
       return true;
     }
     case Intrinsic::cttz:
     case Intrinsic::ctlz:
       // If counting zeros is expensive, try to avoid it.
       return despeculateCountZeros(II, TLI, DL, ModifiedDT);
+    case Intrinsic::dbg_value:
+      return fixupDbgValue(II);
     }
 
     if (TLI) {
       SmallVector<Value*, 2> PtrOps;
       Type *AccessTy;
       if (TLI->getAddrModeArguments(II, PtrOps, AccessTy))
         while (!PtrOps.empty()) {
           Value *PtrVal = PtrOps.pop_back_val();
for (auto &I : reverse(BB)) {
       }
     }
   }
   MadeChange |= dupRetToEnableTailCallOpts(&BB, ModifiedDT);
 
   return MadeChange;
 }
 
+// Some CGP optimizations may move or alter what's computed in a block. Check
+// whether a dbg.value intrinsic could be pointed at a more appropriate operand.
+bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
+  assert(isa<DbgValueInst>(I));
+  DbgValueInst &DVI = *cast<DbgValueInst>(I);
+
+  // Does this dbg.value refer to a sunk address calculation?
+  Value *Location = DVI.getVariableLocation();
+  WeakTrackingVH SunkAddrVH = SunkAddrs[Location];
+  Value *SunkAddr = SunkAddrVH.pointsToAliveValue() ? SunkAddrVH : nullptr;
+  if (SunkAddr) {
+    // Point dbg.value at locally computed address, which should give the best
+    // opportunity to be accurately lowered. This update may change the type of
+    // pointer being referred to; however this makes no difference to debugging
+    // information, and we can't generate bitcasts that may affect codegen.
+    DVI.setOperand(0, MetadataAsValue::get(DVI.getContext(),
+                                           ValueAsMetadata::get(SunkAddr)));
+    return true;
+  }
+  return false;
+}
+
 // llvm.dbg.value is far away from the value then iSel may not be able
 // handle it properly. iSel will drop llvm.dbg.value if it can not
 // find a node corresponding to the value.
 bool CodeGenPrepare::placeDbgValues(Function &F) {
   bool MadeChange = false;
   for (BasicBlock &BB : F) {
     Instruction *PrevNonDbgInst = nullptr;
     for (BasicBlock::iterator BI = BB.begin(), BE = BB.end(); BI != BE;) {

llvm/test/DebugInfo/Generic/codegenprep-addrsink.ll

+; RUN: llc -start-before=codegenprepare -stop-after=codegenprepare %s -o - | FileCheck %s
+;
+; CGP duplicates address calculation into each basic block that contains loads
+; or stores, so that they can be folded into instruction memory operands for
+; example. dbg.value's should be redirected to identify such local address
+; computations, to give the best opportunity for variable locations to be
+; preserved.
+; This test has two dbg.values in it, one before and one after the relevant
+; memory instruction. Test that the one before does _not_ get updated (as that
+; would either make it use-before-def or shift when the variable appears), and
+; that the dbg.value after the memory instruction does get updated.
+;
+; Due to placeDbgValues, the dbg.values currently get shifted up a few
+; instructions.
+
+define dso_local i8 @foo(i32 *%p, i32 %cond) !dbg !7 {
+entry:
+; The first dbg.value of %arith, in the 'next' block, will be moved up here
+; by placeDbgValues,
+; CHECK-LABEL: entry:
+; CHECK:       dbg.value(metadata i8* %arith, metadata ![[DIVAR:[0-9]+]],
+; CHECK-SAME:    metadata !DIExpression()
+  %casted = bitcast i32 *%p to i8*
+  %arith = getelementptr i8, i8 *%casted, i32 3
+  %load1 = load i8, i8 *%arith
+  %cmpresult = icmp eq i32 %cond, 0
+  br i1 %cmpresult, label %next, label %ret
+
+next:
+; Address calcs should be duplicated into this block. One dbg.value should be
+; updated, and the other should not.
+; CHECK-LABEL: next:
+; CHECK:       %[[CASTVAR:[0-9a-zA-Z]+]] = bitcast i32* %p to i8*
+; CHECK-NEXT:  %[[GEPVAR:[0-9a-zA-Z]+]] = getelementptr i8, i8* %[[CASTVAR]],
+; CHECK-SAME:                             i64 3
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i8* %[[GEPVAR]],
+; CHECK-SAME:                            metadata ![[DIVAR]],
+; CHECK-NEXT:  %loaded = load i8, i8* %[[GEPVAR]]
+  call void @llvm.dbg.value(metadata i8 *%arith, metadata !12, metadata !DIExpression()), !dbg !14
+  %loaded = load i8, i8 *%arith
+  call void @llvm.dbg.value(metadata i8 *%arith, metadata !12, metadata !DIExpression()), !dbg !14
+  ret i8 %loaded
+
+ret:
+  ret i8 0
+}
+
+; CHECK: ![[DIVAR]] = !DILocalVariable(name: "p",
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: ".")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 8.0.0 (trunk 348209)"}
+!7 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 4, type: !8, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12}
+!12 = !DILocalVariable(name: "p", arg: 1, scope: !7, file: !1, line: 4, type: !10)
+!14 = !DILocation(line: 4, column: 15, scope: !7)
+!20 = distinct !DILexicalBlock(scope: !7, file: !1, line: 8, column: 7)