[Debuginfo][mem2reg] (6/7) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy).

[alok]

Summary

This patch stems from D84112.
This patch includes changes for mem2reg optimization.

[jmorse]

Suggested on llvm-dev: I reckon you also need to add logic to the ConvertDebugDeclareToDebugValue helpers too, or otherwise factor them in. This patch only covers the two fast-cases that mem2reg considers, single store allocas and single block allocas, I believe (98%) everything else goes through ConvertDebugDeclareToDebugValue.

The tests should also be IR tests that run a single pass (mem2reg) and ensure they do the right thing for a fixed IR input; anything else is vulnerable to changes elsewhere in the compiler.

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp

436

All this new code added will need comments added explaining the intention behind it; it's hard to review otherwise.

449–452

This is a side-note rather than a review comment:

As I understand it, this is ensuring that if someone writes:

dbg.value(%ptr, !123, !DIExpression(DW_OP_deref))

Then we just make the variable refer to the promoted value, rather than making it an implicit pointer. IMO, this isn't something we truly need to support, as a dbg.value like that is is already faulty, see PR40628 [0], they're vulnerable to store re-ordering.

(The overall solution to DW_OP_deref in dbg.values is probably to have the IR verifyer deny them).

https://bugs.llvm.org/show_bug.cgi?id=40628

562–567

StoresByIndex isn't modified after creation; why does it need to be cleared and re-created here?

592

(The disabled code here isn't for review, right)?

616–634

It seems a fair amount of this is shared with the single-value promoter, it can probably be moved to a helper function.

635

It's unclear what the code below is doing without a comment.

llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_instcomb.c

2–3

As with patch six, we can't call clang from llvm tests, this needs to be tested by using "opt" on unoptimized IR. Because this is testing an IR pass, it should test that the correct LLVM-IR output is produced, not an object file.

I agree there should be some kind of end-to-end test; as said in patch six, this is best done in debug-info / Dexter tests, where we can ensure that the debugging experience is as expected, without being too specific about how it's done (which introduces a maintenence burden).

[alok]

Updated to re-base.

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp

class LargeBlockInfo {
   /// the block.
   DenseMap<const Instruction *, unsigned> InstNumbers;
 
 public:
 
   /// This code only looks at accesses to allocas.
   static bool isInterestingInstruction(const Instruction *I) {
     return (isa<LoadInst>(I) && isa<AllocaInst>(I->getOperand(0))) ||
+           (isa<DbgValueInst>(I)) ||
            (isa<StoreInst>(I) && isa<AllocaInst>(I->getOperand(1)));
   }
 
   /// Get or calculate the index of the specified instruction.
   unsigned getInstructionIndex(const Instruction *I) {
     assert(isInterestingInstruction(I) &&
-           "Not a load/store to/from an alloca?");
+           "Not a dbg.value or load/store to/from an alloca?");
 
     // If we already have this instruction number, return it.
     DenseMap<const Instruction *, unsigned>::iterator It = InstNumbers.find(I);
     if (It != InstNumbers.end())
       return It->second;
 
     // Scan the whole block to get the instruction.  This accumulates
     // information for every interesting instruction in the block, in order to
for (DbgVariableIntrinsic *DII : Info.DbgUsers) {
     if (DII->isAddressOfVariable()) {
       DIBuilder DIB(*AI->getModule(), /*AllowUnresolved*/ false);
       ConvertDebugDeclareToDebugValue(DII, Info.OnlyStore, DIB);
       DII->eraseFromParent();
     } else if (DII->getExpression()->startsWithDeref()) {
       DII->eraseFromParent();
     }
   }
+
+  if (AI->getModule()->getDwarfVersion() >= 5) {
+    SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
+    AI->findDbgUsers(DbgUsers);
+    for (auto *DII : DbgUsers) {
+      if (!isa<DbgValueInst>(DII))
+        continue;
+      DIBuilder DIB(*AI->getModule(), /*AllowUnresolved*/ false);
+      auto *DIVar = DII->getVariable();
+      auto *DIExpr = DII->getExpression();
+      SmallVector<uint64_t, 8> Ops;
+      DIExpression::expr_op_iterator Itr = DIExpr->expr_op_begin();
+      DIExpression::expr_op_iterator EndItr = DIExpr->expr_op_end();
+      if (Itr != EndItr && Itr->getOp() == dwarf::DW_OP_deref)
+        Itr++;
+      else
+        Ops.push_back(dwarf::DW_OP_LLVM_implicit_pointer);
+      for (; Itr != EndItr; Itr++) {
+        Itr->appendToVector(Ops);
+      }
+      DIExpr = DIExpression::get(DIExpr->getContext(), Ops);
+      Value *DV = Info.OnlyStore->getValueOperand();
+      DebugLoc DeclareLoc = DII->getDebugLoc();
+      MDNode *Scope = DeclareLoc.getScope();
+      DILocation *InlinedAt = DeclareLoc.getInlinedAt();
+      DebugLoc NewLoc = DebugLoc::get(0, 0, Scope, InlinedAt);
+
+      DIB.insertDbgValueIntrinsic(DV, DIVar, DIExpr, NewLoc, DII);
+      DII->eraseFromParent();
+      LBI.deleteValue(DII);
+    }
+  }
+
   // Remove the (now dead) store and alloca.
   Info.OnlyStore->eraseFromParent();
   LBI.deleteValue(Info.OnlyStore);
 
   AI->eraseFromParent();
   return true;
 }
 
if (I == StoresByIndex.begin()) {
 
       LI->replaceAllUsesWith(ReplVal);
     }
 
     LI->eraseFromParent();
     LBI.deleteValue(LI);
   }
 
+  if (AI->getModule()->getDwarfVersion() >= 5) {
+    StoresByIndex.clear();
+    LBI.clear();
+
+    for (User *U : AI->users())
+      if (StoreInst *SI = dyn_cast<StoreInst>(U))
+        StoresByIndex.push_back(
+            std::make_pair(LBI.getInstructionIndex(SI), SI));
+
+    llvm::sort(StoresByIndex, less_first());
+
+    using DbgValueByIndexTy =
+        SmallVector<std::pair<unsigned, DbgValueInst *>, 64>;
+    DbgValueByIndexTy DbgValueByIndex;
+
+    for (Instruction &I : *(AI->getParent()))
+      if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I))
+        DbgValueByIndex.push_back(
+            std::make_pair(LBI.getInstructionIndex(DVI), DVI));
+
+    llvm::sort(DbgValueByIndex, less_first());
+
+    for (auto User : DbgValueByIndex) {
+      DbgValueInst *DI = User.second;
+
+      if (AI != DI->getValue())
+        continue;
+
+      unsigned Idx = LBI.getInstructionIndex(DI);
+      // Find the nearest store that has a lower index than DI.
+      StoresByIndexTy::iterator I = llvm::lower_bound(
+          StoresByIndex, std::make_pair(Idx, static_cast<StoreInst *>(nullptr)),
+          less_first());
+      if (I == StoresByIndex.begin()) {
+#if 0 // do nothing
+  if (StoresByIndex.empty())
+    // If there are no stores, the load takes the undef value.
+    DI->replaceAllUsesWith(UndefValue::get(LI->getType()));
+  else
+    // There is no store before this load, bail out (load may be affected
+    // by the following stores - see main comment).
+    return false;
+#endif
+      } else {
+        // Otherwise, there was a store before this load, the load takes its
+        // value. Note, if the load was marked as nonnull we don't want to lose
+        // that information when we erase it. So we preserve it with an assume.
+        Value *ReplVal = std::prev(I)->second->getOperand(0);
+#if 0
+  if (AC && LI->getMetadata(LLVMContext::MD_nonnull) &&
+      !isKnownNonZero(ReplVal, DL, 0, AC, LI, &DT))
+    addAssumeNonNull(AC, LI);
+
+  // If the replacement value is the load, this must occur in unreachable
+  // code.
+  if (ReplVal == LI)
+    ReplVal = UndefValue::get(LI->getType());
+#endif
+        DIBuilder DIB(*AI->getModule(), /*AllowUnresolved*/ false);
+        auto *DIVar = DI->getVariable();
+        auto *DIExpr = DI->getExpression();
+        SmallVector<uint64_t, 8> Ops;
+        DIExpression::expr_op_iterator Itr = DIExpr->expr_op_begin();
+        DIExpression::expr_op_iterator EndItr = DIExpr->expr_op_end();
+        if (Itr != EndItr && Itr->getOp() == dwarf::DW_OP_deref)
+          Itr++;
+        else
+          Ops.push_back(dwarf::DW_OP_LLVM_implicit_pointer);
+        for (; Itr != EndItr; Itr++) {
+          Itr->appendToVector(Ops);
+        }
+        DIExpr = DIExpression::get(DIExpr->getContext(), Ops);
+        DebugLoc DeclareLoc = DI->getDebugLoc();
+        MDNode *Scope = DeclareLoc.getScope();
+        DILocation *InlinedAt = DeclareLoc.getInlinedAt();
+        DebugLoc NewLoc = DebugLoc::get(0, 0, Scope, InlinedAt);
+        DIB.insertDbgValueIntrinsic(ReplVal, DIVar, DIExpr, NewLoc, DI);
+
+        DbgValueByIndexTy::iterator Iter = llvm::lower_bound(
+            DbgValueByIndex,
+            std::make_pair(Idx, static_cast<DbgValueInst *>(nullptr)),
+            less_first());
+
+        StoresByIndexTy::iterator EI = StoresByIndex.end();
+        while (++Iter != DbgValueByIndex.end()) {
+          if (DI->getOperand(1) == Iter->second->getOperand(1)) {
+            EI = llvm::lower_bound(
+                StoresByIndex,
+                std::make_pair(Iter->first, static_cast<StoreInst *>(nullptr)),
+                less_first());
+            break;
+          }
+        }
+
+        DI->eraseFromParent();
+        LBI.deleteValue(DI);
+
+        while (I != EI) {
+          ReplVal = I->second->getOperand(0);
+          DIB.insertDbgValueIntrinsic(ReplVal, DIVar, DIExpr,
+                                      I->second->getDebugLoc(), I->second);
+          I++;
+        }
+      }
+    }
+  }
+
   // Remove the (now dead) stores and alloca.
   while (!AI->use_empty()) {
     StoreInst *SI = cast<StoreInst>(AI->user_back());
     // Record debuginfo for the store before removing it.
     for (DbgVariableIntrinsic *DII : Info.DbgUsers) {
       if (DII->isAddressOfVariable()) {
         DIBuilder DIB(*AI->getModule(), /*AllowUnresolved*/ false);
         ConvertDebugDeclareToDebugValue(DII, SI, DIB);

llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_instcomb.c

+// RUN: clang %s -O2 -gdwarf-5 -o %t.o
+// RUN: llvm-dwarfdump  %t.o | FileCheck %s
+
+// CHECK-LABEL:  DW_AT_name    ("__implicit_ptr_tmp_0")
+// CHECK-LABEL:  DW_TAG_variable
+// CHECK-NEXT:  DW_AT_location
+// CHECK-NEXT:  DW_OP_implicit_pointer {{0x.+}} +0
+
+// CHECK-LABEL:  DW_TAG_formal_parameter
+// CHECK-NEXT:  DW_AT_location
+// CHECK-NEXT:  DW_OP_implicit_pointer {{0x.+}} +0
+// CHECK-NEXT:  "ptr"
+
+volatile int gvar = 7;
+
+int func(int *ptr) {
+  gvar = *ptr;
+  return *ptr + 5;
+}
+
+int main() {
+  int var = 4;
+  int *ptrVar = &var;
+
+  int res = func(ptrVar);
+
+  return res;
+}

llvm/test/DebugInfo/X86/implicit_pointer_instcomb.c

+// RUN: clang %s -O2 -gdwarf-5 -S -emit-llvm -o %t.ll
+// RUN: FileCheck %s --input-file=%t.ll
+
+volatile int gvar = 7;
+
+int func(int *ptr) {
+  gvar = *ptr;
+  return *ptr + 5;
+}
+
+int main() {
+  int var = 4;
+  int *ptrVar = &var;
+
+  // CHECK:  call void @llvm.dbg.value(metadata i32 4, metadata [[MD2:!.*]], metadata !DIExpression())
+  // CHECK:  call void @llvm.dbg.value(metadata i32 4, metadata [[MD1:!.*]], metadata !DIExpression(DW_OP_LLVM_implicit_pointer))
+  // CHECK:  call void @llvm.dbg.value(metadata i32 4, metadata [[MD3:!.*]], metadata !DIExpression(DW_OP_LLVM_implicit_pointer))
+  int res = func(ptrVar);
+
+  return res;
+}
+// CHECK: [[MD3]] = !DILocalVariable(name: "ptr"
+// CHECK: [[MD2]] = !DILocalVariable(name: "var"
+// CHECK: [[MD1]] = !DILocalVariable(name: "ptrVar"

llvm/test/DebugInfo/X86/implicit_pointer_promote2reg.c

+// RUN: clang %s -O2 -gdwarf-5 -S -emit-llvm -o %t.ll
+// RUN: FileCheck %s --input-file=%t.ll
+
+static const char *b = "opq";
+volatile int v;
+int main() {
+  int var1 = 4;
+  int var2 = 5;
+  int arr1[2] = {2, 3};
+  int *ptr1;
+  int *ptr2 = 0;
+  int **ptrptr1;
+  int **ptrptr2 = 0;
+
+  v++;
+  ptr1 = &var1;
+  ptr2 = &var2;
+  // CHECK: call void @llvm.dbg.value(metadata i32 4, metadata [[MD1:!.+]], metadata !DIExpression(DW_OP_LLVM_implicit_pointer))
+  // CHECK: call void @llvm.dbg.value(metadata i32 5, metadata [[MD2:!.+]], metadata !DIExpression(DW_OP_LLVM_implicit_pointer))
+  ptrptr1 = &ptr1;
+  ptrptr2 = &ptr2;
+  // CHECK: call void @llvm.dbg.value(metadata i32 4, metadata [[MD3:!.+]], metadata !DIExpression(DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_implicit_pointer))
+  // CHECK: call void @llvm.dbg.value(metadata i32 5, metadata [[MD4:!.+]], metadata !DIExpression(DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_implicit_pointer))
+
+  v++;
+
+  return arr1[0] + arr1[1] + *ptr1 + *ptr2 + **ptrptr1 + **ptrptr2 - 5;
+}
+// CHECK: [[MD1]] = !DILocalVariable(name: "ptr1"
+// CHECK: [[MD2]] = !DILocalVariable(name: "ptr2"
+// CHECK: [[MD3]] = !DILocalVariable(name: "ptrptr1"
+// CHECK: [[MD4]] = !DILocalVariable(name: "ptrptr2"

llvm/test/DebugInfo/X86/implicit_pointer_temp_reference.cc

+// RUN: clang %s -O2 -gdwarf-5 -S -emit-llvm -o %t.ll
+// RUN: FileCheck %s --input-file=%t.ll
+
+__attribute__((optnone)) int source() {
+  return 3;
+}
+__attribute__((optnone)) void f(int i) {
+}
+inline void sink(const int &p) {
+  f(p);
+}
+int main() {
+  // CHECK:  call void @llvm.dbg.value(metadata i32 3, metadata [[MD:!.+]], metadata !DIExpression(DW_OP_LLVM_implicit_pointer))
+  sink(source());
+}
+// CHECK: [[MD]] = !DILocalVariable(name: "p"