[Utils] Insert DW_OP_bit_piece when only describing part of the variable

[loladiro]

Summary

The dbg.declare -> dbg.value conversion looks through any zext/sext to find a value to describe the variable (in the expectation that those zext/sext instruction will go away later). However, those values do not cover the entire variable and thus need a DW_OP_bit_piece.

[aprantl]

lib/Transforms/Utils/Local.cpp

1061 ↗

(On Diff #44474)

It's not clear to me that a sign/zero-extended value always indicates that the value is a subset of the variable. Is that guaranteed? Note that the Verifier checks that a piece does not cover the entire variable.

test/Transforms/Util/split-bit-piece.ll

1 ↗

(On Diff #44474)

SORA can create bit pieces, but in this case I assume it's Local that is supposed to generate the bit piece?

14 ↗

(On Diff #44474)

Please also check for the fist argument of the dbg.value so it is clear what the assumptions about the lowering are. This will help us keeping the test useful in the future.

[loladiro]

lib/Transforms/Utils/Local.cpp

1061 ↗

(On Diff #44474)

Yes, I think that's guaranteed. sext/zext guarantee that they expand from a smaller to a larger value, and if the IR was correct before, then the extended value would cover the entire variable, so this should be smaller. One case that does occur to me however, is where the expression already contains a DW_OP_bit_piece. In that case we'll have to update the existing one.

test/Transforms/Util/split-bit-piece.ll

1 ↗

(On Diff #44474)

Well, SORA calls the Local util, but I'll make the comment more clear.

14 ↗

(On Diff #44474)

Ok.

[loladiro]

• Fix the case where the expression already is a bit piece
• Update comment explaining what the test is testing
• Check for the first argument of the llvm.dbg.value call

[aprantl]

Accepted with minor comments.

lib/Transforms/Utils/Local.cpp

1056 ↗

(On Diff #44553)

Please add a comment explaining why and why the piece has to be smaller than the variable.
Nitpick: the clang coding standards prefer full sentences as comments, with a trailing '.'.

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

bool llvm::ConvertDebugDeclareToDebugValue(DbgDeclareInst *DDI,
 
   // If an argument is zero extended then use argument directly. The ZExt
   // may be zapped by an optimization pass in future.
   Argument *ExtendedArg = nullptr;
   if (ZExtInst *ZExt = dyn_cast<ZExtInst>(SI->getOperand(0)))
     ExtendedArg = dyn_cast<Argument>(ZExt->getOperand(0));
   if (SExtInst *SExt = dyn_cast<SExtInst>(SI->getOperand(0)))
     ExtendedArg = dyn_cast<Argument>(SExt->getOperand(0));
-  if (ExtendedArg)
-    Builder.insertDbgValueIntrinsic(ExtendedArg, 0, DIVar, DIExpr,
+  if (ExtendedArg) {
+    // We're now only describing a subset of the variable. The piece we're
+    // describing will always be smaller than the variable size, because
+    // VariableSize == Size of Alloca described by DDI. Since SI stores
+    // to the alloca described by DDI, if it's first operand is an extend,
+    // we're guaranteed that before extension, the value was narrower than
+    // the size of the alloca, hence the size of the described variable.
+    SmallVector<uint64_t, 3> NewDIExpr;
+    unsigned PieceOffset = 0;
+    // If this already is a bit piece, we drop the bit piece from the expression
+    // and record the offset.
+    if (DIExpr->isBitPiece()) {
+      NewDIExpr.append(DIExpr->elements_begin(), DIExpr->elements_end()-3);
+      PieceOffset = DIExpr->getBitPieceOffset();
+    } else {
+      NewDIExpr.append(DIExpr->elements_begin(), DIExpr->elements_end());
+    }
+    NewDIExpr.push_back(dwarf::DW_OP_bit_piece);
+    NewDIExpr.push_back(PieceOffset); //Offset
+    const DataLayout &DL = DDI->getModule()->getDataLayout();
+    NewDIExpr.push_back(DL.getTypeSizeInBits(ExtendedArg->getType())); // Size
+    Builder.insertDbgValueIntrinsic(ExtendedArg, 0, DIVar,
+                                    Builder.createExpression(NewDIExpr),
                                     DDI->getDebugLoc(), SI);
+  }
   else
     Builder.insertDbgValueIntrinsic(SI->getOperand(0), 0, DIVar, DIExpr,
                                     DDI->getDebugLoc(), SI);
   return true;
 }
 
 /// Inserts a llvm.dbg.value intrinsic before a load of an alloca'd value
 /// that has an associated llvm.dbg.decl intrinsic.

llvm/trunk/test/Transforms/Util/split-bit-piece.ll

+; Checks that llvm.dbg.declare -> llvm.dbg.value conversion utility
+; (here exposed through the SROA) pass, properly inserts bit_piece expressions
+; if it only describes part of the variable.
+; RUN: opt -S -sroa %s | FileCheck %s
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #0
+
+; Function Attrs: nounwind uwtable
+define hidden void @_ZN6__tsan9FastState14SetHistorySizeEi(i32 %hs) #1 align 2 {
+entry:
+  %hs.addr = alloca i32, align 4
+  %v1 = alloca i64, align 8
+  %v2 = alloca i64, align 8
+  store i32 %hs, i32* %hs.addr, align 4
+; CHECK: call void @llvm.dbg.value(metadata i32 %hs, i64 0, metadata !{{[0-9]+}}, metadata ![[EXPR:[0-9]+]])
+; CHECK: ![[EXPR]] = !DIExpression(DW_OP_bit_piece, 0
+  call void @llvm.dbg.declare(metadata i64* %v1, metadata !9, metadata !12), !dbg !13
+  %0 = load i32, i32* %hs.addr, align 4
+  %conv = sext i32 %0 to i64
+  store i64 %conv, i64* %v1, align 8
+  %1 = load i64, i64* %v2, align 8
+  unreachable
+}
+
+attributes #0 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.8.0 (trunk 256979) (llvm/trunk 257107)", isOptimized: false, runtimeVersion: 0, emissionKind: 1, retainedTypes: !2)
+!1 = !DIFile(filename: "tsan_shadow_test.cc", directory: "/tmp")
+!2 = !{!3, !5}
+!3 = !DICompositeType(tag: DW_TAG_class_type, name: "FastState", file: !4, line: 91, size: 64, align: 64, identifier: "_ZTSN6__tsan9FastStateE")
+!4 = !DIFile(filename: "/mnt/extra/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_rtl.h", directory: "/tmp")
+!5 = distinct !DIDerivedType(tag: DW_TAG_typedef, name: "u64", line: 78, baseType: !6)
+!6 = !DIBasicType(name: "long long unsigned int", size: 64, align: 64, encoding: DW_ATE_unsigned)
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!8 = !{!"clang version 3.8.0 (trunk 256979) (llvm/trunk 257107)"}
+!9 = !DILocalVariable(name: "v1", scope: !10, file: !4, line: 136, type: !5)
+!10 = distinct !DILexicalBlock(scope: !11, file: !4, line: 136, column: 5)
+!11 = distinct !DISubprogram(name: "SetHistorySize", linkageName: "_ZN6__tsan9FastState14SetHistorySizeEi", scope: !"_ZTSN6__tsan9FastStateE", file: !4, line: 135, isLocal: false, isDefinition: true, scopeLine: 135, flags: DIFlagPrototyped, isOptimized: false)
+!12 = !DIExpression()
+!13 = !DILocation(line: 136, column: 5, scope: !10)