[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

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

2

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

15

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

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

2

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

15

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

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 '.'.

lib/Transforms/Utils/Local.cpp

     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
+    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);

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)