[mem2reg][debuginfo] Handle op_deref when converting dbg.declare

[fdeazeve]

Summary

The conversion of dbg.declare into dbg.values doesn't take into account
the DIExpression attached to the intrinsic. In particular, when
converting:

store %val, ptr %alloca
dbg.declare(ptr %alloca, !SomeVar, !DIExpression())

Mem2Reg will try to figure out if %val has the size of !SomeVar. If
it does, then a non-undef dbg.value is inserted:

dbg.value(%val, !SomeVar, !DIExpression())

This makes sense: the alloca is _the_ address of the variable. So a
store to the alloca is a store to the variable. However, if the
expression in the original intrinsic is a DW_OP_deref, this logic is
not applicable:

store ptr %val, ptr %alloca
dbg.declare(ptr %alloca, !SomeVar, !DIExpression(DW_OP_deref))

Here, the alloca is *not* the address of the variable. A store to the
alloca is *not* a store to the variable. As such, querying whether
%val has the same size as !SomeVar is meaningless.

This patch addresses the issue by:

1. Allowing the conversion when the expression is _only_ a DW_OP_deref

without any other expressions (see code comment).

2. Checking that the expression does not start with a DW_OP_deref

before applying the logic that checks whether the value being stored and
the variable have the same length.

[aprantl]

llvm/lib/Transforms/Utils/Local.cpp

1534

I'm not sure the second half of this sentence is necessarily true, I think it's more an emergent behavior of the instruction selector, because at the LLVM IR level we don't really distinguish what DWARF calls location description kinds, but it certainly correct that the two are not equivalent.

One thing you could do is define the semantics of dbg.declare(alloca, var, DIExpression(DW_OP_deref)) as the alloca holding the canonical address of var, *which may not change throughout the entire function*. Patching LangRef this way would allow you to do the kind of transformation that you need here.

I'm also curious if how we defined llvm.dbg.addr() and if it would be of any use here.

[Orlando]

llvm/lib/Transforms/Utils/Local.cpp

1534

+ to everything @aprantl said. Note that dbg.addr may be on the way out - see this discourse post.

I could be wrong but I don't think the dbg.value in the example looks quite right - IIRC it should end with either a stack_value or deref operand if the expression is non-empty because dbg.values are essentially implicit location descriptions (describing the value of the variable rather than its location).

[fdeazeve]

Updated comment to address review comments, rebased.

llvm/lib/Transforms/Utils/Local.cpp

1534

I could be wrong but I don't think the dbg.value in the example looks quite right

It is indeed intentionally wrong, the comment was attempting to explain why doing the conversion of dbg.declare -> dbg.value would be incorrect in that case. I've updated the comment.

I'm also curious if how we defined llvm.dbg.addr() and if it would be of any use here.

As per the link Orlando posted, the intrinsic is indeed going away, and the recommendation is to replace dbg.addr with a dbg.value + op_deref (which is what is being generated here!).

[fdeazeve]

This should now be ready for review

[Orlando]

Sorry for the slow turn around on this review - LGTM too, thanks!

llvm/include/llvm/IR/DebugInfoMetadata.h

2772

nit: exactly one operator / expression operator / operation etc?

[fdeazeve]

In D142160#4090201, @Orlando wrote:

nit: exactly one operator / expression operator / operation etc?

Great point! Will fix

[fdeazeve]

Rebased and updated comment in header file.

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
 
   static bool classof(const Metadata *MD) {
     return MD->getMetadataID() == DIExpressionKind;
   }
 
   /// Return whether the first element a DW_OP_deref.
   bool startsWithDeref() const;
 
+  /// Return whether there is exactly one operator and it is a DW_OP_deref;
+  bool isDeref() const;
+
   /// Holds the characteristics of one fragment of a larger variable.
   struct FragmentInfo {
     uint64_t SizeInBits;
     uint64_t OffsetInBits;
   };
 
   /// Retrieve the details of this fragment expression.
   static std::optional<FragmentInfo> getFragmentInfo(expr_op_iterator Start,

llvm/lib/IR/DebugInfoMetadata.cpp

DIExpression *DIExpression::getImpl(LLVMContext &Context,
   DEFINE_GETIMPL_STORE_NO_OPS(DIExpression, (Elements));
 }
 bool DIExpression::isEntryValue() const {
   return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_LLVM_entry_value;
 }
 bool DIExpression::startsWithDeref() const {
   return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_deref;
 }
+bool DIExpression::isDeref() const {
+  return getNumElements() == 1 && startsWithDeref();
+}
 
 DIAssignID *DIAssignID::getImpl(LLVMContext &Context, StorageType Storage,
                                 bool ShouldCreate) {
   // Uniqued DIAssignID are not supported as the instance address *is* the ID.
   assert(Storage != StorageType::Uniqued && "uniqued DIAssignID unsupported");
   return storeImpl(new (0u, Storage) DIAssignID(Context, Storage), Storage);
 }
 

llvm/lib/Transforms/Utils/Local.cpp

void llvm::ConvertDebugDeclareToDebugValue(DbgVariableIntrinsic *DII,
   assert(DII->isAddressOfVariable() || isa<DbgAssignIntrinsic>(DII));
   auto *DIVar = DII->getVariable();
   assert(DIVar && "Missing variable");
   auto *DIExpr = DII->getExpression();
   Value *DV = SI->getValueOperand();
 
   DebugLoc NewLoc = getDebugValueLoc(DII);
 
-  if (!valueCoversEntireFragment(DV->getType(), DII)) {
-    // FIXME: If storing to a part of the variable described by the dbg.declare,
-    // then we want to insert a dbg.value for the corresponding fragment.
-    LLVM_DEBUG(dbgs() << "Failed to convert dbg.declare to dbg.value: "
-                      << *DII << '\n');
-    // For now, when there is a store to parts of the variable (but we do not
-    // know which part) we insert an dbg.value intrinsic to indicate that we
-    // know nothing about the variable's content.
-    DV = UndefValue::get(DV->getType());
-    Builder.insertDbgValueIntrinsic(DV, DIVar, DIExpr, NewLoc, SI);
-    return;
-  }
-
-  Builder.insertDbgValueIntrinsic(DV, DIVar, DIExpr, NewLoc, SI);
+  // If the alloca describes the variable itself, i.e. the expression in the
+  // dbg.declare doesn't start with a dereference, we can perform the
+  // conversion if the value covers the entire fragment of DII.
+  // If the alloca describes the *address* of DIVar, i.e. DIExpr is
+  // *just* a DW_OP_deref, we use DV as is for the dbg.value.
+  // We conservatively ignore other dereferences, because the following two are
+  // not equivalent:
+  //     dbg.declare(alloca, ..., !Expr(deref, plus_uconstant, 2))
+  //     dbg.value(DV, ..., !Expr(deref, plus_uconstant, 2))
+  // The former is adding 2 to the address of the variable, whereas the latter
+  // is adding 2 to the value of the variable. As such, we insist on just a
+  // deref expression.
+  bool CanConvert =
+      DIExpr->isDeref() || (!DIExpr->startsWithDeref() &&
+                            valueCoversEntireFragment(DV->getType(), DII));
+  if (CanConvert) {
+    Builder.insertDbgValueIntrinsic(DV, DIVar, DIExpr, NewLoc, SI);
+    return;
+  }
+
+  LLVM_DEBUG(dbgs() << "Failed to convert dbg.declare to dbg.value: " << *DII
+                    << '\n');
 }
 
 /// Inserts a llvm.dbg.value intrinsic before a load of an alloca'd value
 /// that has an associated llvm.dbg.declare or llvm.dbg.addr intrinsic.
 void llvm::ConvertDebugDeclareToDebugValue(DbgVariableIntrinsic *DII,
                                            LoadInst *LI, DIBuilder &Builder) {
   auto *DIVar = DII->getVariable();
   auto *DIExpr = DII->getExpression();

llvm/test/Transforms/Mem2Reg/dbg_declare_to_value_conversions.ll

+; RUN: opt < %s -passes='mem2reg' -S | FileCheck %s
+target datalayout = "e-p:64:64"
+
+; An intrinsic without any expressions should always be converted.
+define i64 @foo0(i64 %arg) {
+  %arg.addr = alloca i64
+  store i64 %arg, ptr %arg.addr
+  call void @llvm.dbg.declare(metadata ptr %arg.addr, metadata !26, metadata !DIExpression()), !dbg !40
+  ; CHECK-LABEL: @foo0
+  ; CHECK-SAME:    (i64 [[arg:%.*]])
+  ; CHECK-NEXT:    dbg.value(metadata i64 [[arg]], {{.*}}, metadata !DIExpression())
+  %val = load i64, ptr %arg.addr
+  ret i64 %val
+}
+
+; An intrinsic with a single deref operator should be converted preserving the deref.
+define i32 @foo1(ptr %arg) {
+  %arg.indirect_addr = alloca ptr
+  store ptr %arg, ptr %arg.indirect_addr
+  call void @llvm.dbg.declare(metadata ptr %arg.indirect_addr, metadata !25, metadata !DIExpression(DW_OP_deref)), !dbg !40
+  ; CHECK-LABEL: @foo1
+  ; CHECK-SAME:    (ptr [[arg:%.*]])
+  ; CHECK-NEXT:    dbg.value(metadata ptr [[arg]], {{.*}}, metadata !DIExpression(DW_OP_deref))
+  %val = load i32, ptr %arg
+  ret i32 %val
+}
+
+
+; An intrinsic with multiple operators should cause us to conservatively bail.
+define i32 @foo2(ptr %arg) {
+  %arg.indirect_addr = alloca ptr
+  store ptr %arg, ptr %arg.indirect_addr
+  call void @llvm.dbg.declare(metadata ptr %arg.indirect_addr, metadata !25, metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 2)), !dbg !40
+  ; CHECK-LABEL: @foo2
+  ; CHECK-NEXT:     dbg.value(metadata ptr undef, {{.*}}, metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 2))
+  %val = load i32, ptr %arg
+  ret i32 %val
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!1, !2}
+!llvm.dbg.cu = !{!7}
+
+!1 = !{i32 7, !"Dwarf Version", i32 4}
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DIBasicType(name: "wide type", size: 512)
+!4 = !DIBasicType(name: "ptr sized type", size: 64)
+!7 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !8, producer: "clang", emissionKind: FullDebug)
+!8 = !DIFile(filename: "test.ll", directory: "")
+!10 = distinct !DISubprogram(name: "blah", linkageName: "blah", scope: !8, file: !8, line: 7, unit: !7)
+!25 = !DILocalVariable(name: "blah", arg: 1, scope: !10, file: !8, line: 7, type:!3)
+!26 = !DILocalVariable(name: "ptr sized var", arg: 1, scope: !10, file: !8, line: 7, type:!4)
+!40 = !DILocation(line: 7, column: 35, scope: !10)