Improve ConvertDebugDeclareToDebugValue

[bjope]

Summary

This is a follow-up to r334830 and r335031.

In the valueCoversEntireFragment check we now also handle
the situation when there is a variable length array (VLA)
involved, and the length of the array has been reduced to
a constant.

The ConvertDebugDeclareToDebugValue functions that are related
to PHI nodes and load instructions now avoid inserting dbg.value
intrinsics when the value does not, for certain, cover the
variable/fragment that should be described.
In r334830 we assumed that the value always covered the entire
var/fragment and we had assertions in the code to show that
assumption. However, those asserts failed when compiling code
with VLAs, so we removed the asserts in r335031. Now when we
know that the valueCoversEntireFragment check can fail also for
PHI/Load instructions we avoid to insert the faulty dbg.value
intrinsic in such situations. Compared to the Store instruction
scenario we simply drop the dbg.value here (as the variable does
not change its value due to PHI/Load, so an earlier dbg.value
describing the variable should still be valid).

[aprantl]

include/llvm/IR/Instructions.h

103 ↗

(On Diff #152685)

We generally try to move away from special values. Would it make sense to return an Optional<uint64_t> here?

test/Transforms/Mem2Reg/debug-alloca-vla-1.ll

3 ↗

(On Diff #152685)

Can you add a comment explaining what is being tested here?

test/Transforms/Mem2Reg/debug-alloca-vla-2.ll

20 ↗

(On Diff #152685)

and here?

[bjope]

Update after review comments:

• Use Optional in AllocaInst::getAllocationSizeInBits.
• Add comments describing the added test cases.

llvm/trunk/include/llvm/IR/Instructions.h

public:
   const Value *getArraySize() const { return getOperand(0); }
   Value *getArraySize() { return getOperand(0); }
 
   /// Overload to return most specific pointer type.
   PointerType *getType() const {
     return cast<PointerType>(Instruction::getType());
   }
 
+  /// Get allocation size in bits. Returns None if size can't be determined,
+  /// e.g. in case of a VLA.
+  Optional<uint64_t> getAllocationSizeInBits(const DataLayout &DL) const;
+
   /// Return the type that is being allocated by the instruction.
   Type *getAllocatedType() const { return AllocatedType; }
   /// for use only in special circumstances that need to generically
   /// transform a whole instruction (eg: IR linking and vectorization).
   void setAllocatedType(Type *Ty) { AllocatedType = Ty; }
 
   /// Return the alignment of the memory that is being allocated by the
   /// instruction.

llvm/trunk/lib/IR/Instructions.cpp

 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <vector>
 
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
+//                            AllocaInst Class
+//===----------------------------------------------------------------------===//
+
+Optional<uint64_t>
+AllocaInst::getAllocationSizeInBits(const DataLayout &DL) const {
+  uint64_t Size = DL.getTypeAllocSizeInBits(getAllocatedType());
+  if (isArrayAllocation()) {
+    auto C = dyn_cast<ConstantInt>(getArraySize());
+    if (!C)
+      return None;
+    Size *= C->getZExtValue();
+  }
+  return Size;
+}
+
+//===----------------------------------------------------------------------===//
 //                            CallSite Class
 //===----------------------------------------------------------------------===//
 
 User::op_iterator CallSite::getCallee() const {
   Instruction *II(getInstruction());
   return isCall()
     ? cast<CallInst>(II)->op_end() - 1 // Skip Callee
     : cast<InvokeInst>(II)->op_end() - 3; // Skip BB, BB, Callee

llvm/trunk/lib/Transforms/Utils/Local.cpp

 /// alloc size of the value when doing the comparison. E.g. an i1 value will be
 /// identified as covering an n-bit fragment, if the store size of i1 is at
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgInfoIntrinsic *DII) {
   const DataLayout &DL = DII->getModule()->getDataLayout();
   uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
   if (auto FragmentSize = DII->getFragmentSizeInBits())
     return ValueSize >= *FragmentSize;
+  // We can't always calculate the size of the DI variable (e.g. if it is a
+  // VLA). Try to use the size of the alloca that the dbg intrinsic describes
+  // intead.
+  if (DII->isAddressOfVariable())
+    if (auto *AI = dyn_cast_or_null<AllocaInst>(DII->getVariableLocation()))
+      if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
+        return ValueSize >= *FragmentSize;
+  // Could not determine size of variable. Conservatively return false.
   return false;
 }
 
 /// Inserts a llvm.dbg.value intrinsic before a store to an alloca'd value
 /// that has an associated llvm.dbg.declare or llvm.dbg.addr intrinsic.
 void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII,
                                            StoreInst *SI, DIBuilder &Builder) {
   assert(DII->isAddressOfVariable());
void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII,
                                            LoadInst *LI, DIBuilder &Builder) {
   auto *DIVar = DII->getVariable();
   auto *DIExpr = DII->getExpression();
   assert(DIVar && "Missing variable");
 
   if (LdStHasDebugValue(DIVar, DIExpr, LI))
     return;
 
+  if (!valueCoversEntireFragment(LI->getType(), DII)) {
+    // FIXME: If only referring 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');
+    return;
+  }
+
   // We are now tracking the loaded value instead of the address. In the
   // future if multi-location support is added to the IR, it might be
   // preferable to keep tracking both the loaded value and the original
   // address in case the alloca can not be elided.
   Instruction *DbgValue = Builder.insertDbgValueIntrinsic(
       LI, DIVar, DIExpr, DII->getDebugLoc(), (Instruction *)nullptr);
   DbgValue->insertAfter(LI);
 }
 
 /// Inserts a llvm.dbg.value intrinsic after a phi that has an associated
 /// llvm.dbg.declare or llvm.dbg.addr intrinsic.
 void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII,
                                            PHINode *APN, DIBuilder &Builder) {
   auto *DIVar = DII->getVariable();
   auto *DIExpr = DII->getExpression();
   assert(DIVar && "Missing variable");
 
   if (PhiHasDebugValue(DIVar, DIExpr, APN))
     return;
 
+  if (!valueCoversEntireFragment(APN->getType(), DII)) {
+    // FIXME: If only referring 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');
+    return;
+  }
+
   BasicBlock *BB = APN->getParent();
   auto InsertionPt = BB->getFirstInsertionPt();
 
   // The block may be a catchswitch block, which does not have a valid
   // insertion point.
   // FIXME: Insert dbg.value markers in the successors when appropriate.
   if (InsertionPt != BB->end())
     Builder.insertDbgValueIntrinsic(APN, DIVar, DIExpr, DII->getDebugLoc(),

llvm/trunk/test/Transforms/Mem2Reg/debug-alloca-vla-1.ll

+; RUN: opt < %s -mem2reg -S | FileCheck %s
+
+; Testing conversion from dbg.declare to dbg.value when the variable is a VLA.
+;
+; We can't derive the size of the variable simply by looking at the
+; metadata. But we can find out the size by examining the alloca, so we should
+; know that the load/store instructions are referencing the whole variable,
+; and we expect to get dbg.value intrinsics that maps %entryN (aka %[[PHI]])
+; and %t0 to the variable allocated as %vla1.
+
+; ModuleID = 'debug-alloca-vla.ll'
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.12.0"
+
+; Function Attrs: nounwind ssp uwtable
+define void @scan() #0 !dbg !4 {
+entry:
+  %vla1 = alloca i32, i32 1, align 8
+  call void @llvm.dbg.declare(metadata i32* %vla1, metadata !10, metadata !DIExpression()), !dbg !18
+  br label %for.cond, !dbg !18
+
+for.cond:                                         ; preds = %for.cond, %entry
+; CHECK: %[[PHI:.*]] = phi i32 [ undef, %entry ], [ %t0, %for.cond ]
+  %entryN = load i32, i32* %vla1, align 8, !dbg !18
+; CHECK: call void @llvm.dbg.value(metadata i32 %[[PHI]],
+; CHECK-SAME:                      metadata !DIExpression())
+  %t0 = add i32 %entryN, 1
+; CHECK: %t0 = add i32 %[[PHI]], 1
+; CHECK: call void @llvm.dbg.value(metadata i32 %t0,
+; CHECK-SAME:                      metadata !DIExpression())
+ store i32 %t0, i32* %vla1, align 8, !dbg !18
+  br label %for.cond, !dbg !18
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind ssp uwtable }
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "adrian", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "<stdin>", directory: "/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !{i32 7, !"PIC Level", i32 2}
+!4 = distinct !DISubprogram(name: "scan", scope: !1, file: !1, line: 4, type: !5, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !8)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null, !7, !7}
+!7 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!8 = !{!9}
+!9 = !DILocalVariable(name: "entry", scope: !4, file: !1, line: 6, type: !7)
+!10 = !DILocalVariable(name: "ptr32", scope: !4, file: !1, line: 240, type: !11)
+!11 = !DICompositeType(tag: DW_TAG_array_type, baseType: !12, elements: !14)
+!12 = !DIDerivedType(tag: DW_TAG_typedef, name: "__uint32_t", file: !1, line: 41, baseType: !13)
+!13 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!14 = !{!15}
+!15 = !DISubrange(count: !16)
+!16 = !DILocalVariable(name: "__vla_expr", scope: !4, type: !17, flags: DIFlagArtificial)
+!17 = !DIBasicType(name: "long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+!18 = !DILocation(line: 6, scope: !4)

llvm/trunk/test/Transforms/Mem2Reg/debug-alloca-vla-2.ll

+; RUN: opt < %s -mem2reg -S | FileCheck %s
+
+; Testing conversion from dbg.declare to dbg.value when the variable is a VLA.
+;
+; We can't derive the size of the variable since it is a VLA with an unknown
+; number of element.
+;
+; Verify that we do not get a dbg.value after the phi node (we can't know if
+; the phi nodes result describes the whole array or not).  Also verify that we
+; get a dbg.value that says that we do not know the value of the VLA in place
+; of the store (since we do not know which part of the VLA the store is
+; writing to).
+
+; ModuleID = 'debug-alloca-vla.ll'
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.12.0"
+
+; Function Attrs: nounwind ssp uwtable
+define void @scan(i32 %n) #0 !dbg !4 {
+entry:
+  %vla1 = alloca i32, i32 %n, align 8
+  call void @llvm.dbg.declare(metadata i32* %vla1, metadata !10, metadata !DIExpression()), !dbg !18
+  br label %for.cond, !dbg !18
+
+for.cond:                                         ; preds = %for.cond, %entry
+; CHECK: %[[PHI:.*]] = phi i32 [ undef, %entry ], [ %t0, %for.cond ]
+  %entryN = load i32, i32* %vla1, align 8, !dbg !18
+; CHECK-NOT: call void @llvm.dbg.value
+  %t0 = add i32 %entryN, 1
+; CHECK: %t0 = add i32 %[[PHI]], 1
+; CHECK: call void @llvm.dbg.value(metadata i32 undef,
+; CHECK-SAME:                      metadata !DIExpression())
+ store i32 %t0, i32* %vla1, align 8, !dbg !18
+  br label %for.cond, !dbg !18
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind ssp uwtable }
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "adrian", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "<stdin>", directory: "/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !{i32 7, !"PIC Level", i32 2}
+!4 = distinct !DISubprogram(name: "scan", scope: !1, file: !1, line: 4, type: !5, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !8)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null, !7, !7}
+!7 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!8 = !{!9}
+!9 = !DILocalVariable(name: "entry", scope: !4, file: !1, line: 6, type: !7)
+!10 = !DILocalVariable(name: "ptr32", scope: !4, file: !1, line: 240, type: !11)
+!11 = !DICompositeType(tag: DW_TAG_array_type, baseType: !12, elements: !14)
+!12 = !DIDerivedType(tag: DW_TAG_typedef, name: "__uint32_t", file: !1, line: 41, baseType: !13)
+!13 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!14 = !{!15}
+!15 = !DISubrange(count: !16)
+!16 = !DILocalVariable(name: "__vla_expr", scope: !4, type: !17, flags: DIFlagArtificial)
+!17 = !DIBasicType(name: "long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+!18 = !DILocation(line: 6, scope: !4)