WIP: [Utils] Salvage debug info of DCE'ed extractvalue instructions

[vsk]

Summary

Based on a recent ToT, this salvages an additional 1072 debug values in a
stage2 build of clang and preserves an additional 40 source variables (as
counted by llvm-dwarfdump --statistics).

WIP: I have not yet found a small, end-to-end test for this patch (i.e something I can verify in a debugger). I'm waiting on delta to reduce a file in llvm that triggers the new logic.

[aprantl]

lib/Transforms/Utils/Local.cpp

1601

Can you add a comment explaining why this is safe? I.e., because the size of the variable being described implicitly masks out the upper bits of the value..

1605

Why does this loop go all the way to FieldIndices.size()?

[aprantl]

lib/Transforms/Utils/Local.cpp

1605

This is overloading I which makes the code harder to read.

1605

got it.. this is iterating over the field indices of the extract_value instruction, not over the data type! I was confused by this.

test/Transforms/InstCombine/debuginfo-variables.ll

124

Hard-coding the metadata numbering (metadata !92, !dbg !100) seems to be really fragile, and probably doesn't add anything to the test.

[vsk]

lib/Transforms/Utils/Local.cpp

1605

I'll add a comment.

test/Transforms/InstCombine/debuginfo-variables.ll

124

It tests that we don't apply debug values to the wrong variables, which seemed worthwhile to me. Can we keep the numbering until it makes test updates harder than they need to be? Or would you rather simply delete them now?

[aprantl]

test/Transforms/InstCombine/debuginfo-variables.ll

124

Inevitably in the near future someone will change instcombine or the debug info in a way that changes the relative order of the MDNodes and then they will have a hard time understanding how to update this testcase. For the same reason it will make cherry-picking this patch to a release branch much harder than necessary. Since the numbering here really isn't relevant to the test, I would prefer to not check for it at all. E.g.:

CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty2 %S,
CHECK-SAME:                                             metadata !DIExpression(DW_OP_constu, 8, DW_OP_shr, DW_OP_stack_value))

That's hoping that combining CHECK-NEXT and CHECK-SAME like this actually works.

[vsk]

I'm having trouble finding an end-to-end test for this because of issues in the register allocator. This may end up depending on https://reviews.llvm.org/D43427 and possibly other follow-ups.

test/Transforms/InstCombine/debuginfo-variables.ll

124

Gotcha, I'll fix this in the next update.

[aprantl]

I have not yet found a small, end-to-end test for this patch

I don't think that one is strictly necessary. Testing the result of the pass directly is preferable anyway.

[vsk]

In D43324#1011509, @aprantl wrote:

I have not yet found a small, end-to-end test for this patch

I don't think that one is strictly necessary. Testing the result of the pass directly is preferable anyway.

I wasn't planning on checking in the end-to-end test -- I just wanted to make sure that it's possible to see variables salvaged from extractvalue instructions in the debugger.

I did some more digging and found that FastISel does not emit DBG_VALUE MI's when it sees the sorts of llvm.dbg.value() instructions this patch generates. Concretely, no DBG_VALUE is generated for this instruction:

%s_ev = load %struct.S, %struct.S* %s
%x_ev = extractvalue %struct.S %s_ev, 0
call void @llvm.dbg.value(metadata %struct.S %s_ev, metadata !24, metadata !DIExpression()), !dbg !55

[dblaikie]

lib/Transforms/Utils/Local.cpp

1625

Maybe this could/should be:

if (auto *StructTy = dyn_cast<StructType(IndexedTy)) {
  ...
} else {
  assert(array type);
  ...
}

lib/Transforms/Utils/Local.cpp

if (auto *CI = dyn_cast<CastInst>(&I)) {
     for (auto *DII : DbgUsers) {
       // Rewrite the load into DW_OP_deref.
       auto *DIExpr = DII->getExpression();
       DIExpr = DIExpression::prepend(DIExpr, DIExpression::WithDeref);
       DII->setOperand(0, AddrMD);
       DII->setOperand(2, MetadataAsValue::get(I.getContext(), DIExpr));
       DEBUG(dbgs() << "SALVAGE:  " << *DII << '\n');
     }
+  } else if (auto *EVI = dyn_cast<ExtractValueInst>(&I)) {
+    // An extractvalue instruction indexes into an aggregate value using one
+    // or more indices. Rewrite the extractvalue by calculating the offset of
+    // the desired value within the aggregate and shifting the aggregate by
+    // that many bits. This is safe because the debug info records the size of
+    // the desired value: excess high bits of the aggregate are discarded. E.g:
+    //
+    //   struct { int32_t x, y, z } s;
+    //   s.y  =>  extractvalue s, 1  =>  s >> 32
+    //
+    // We handle nested aggregates by summing up the offsets to sub-aggregates,
+    // one at a time, until each index in the extractvalue is accounted for.
+    Value *Agg = EVI->getAggregateOperand();
+    ArrayRef<unsigned> Indices = EVI->getIndices();
+    unsigned FieldOffset = 0;
+    for (unsigned Depth = 0, E = Indices.size(); Depth < E; ++Depth) {
+      ArrayRef<unsigned> IndicesForSubAgg = Indices.take_front(Depth);
+      Type *IndexedTy =
+          ExtractValueInst::getIndexedType(Agg->getType(), IndicesForSubAgg);
+      if (IndexedTy->isStructTy()) {
+        auto *Layout = DL.getStructLayout(cast<StructType>(IndexedTy));
+        FieldOffset += Layout->getElementOffsetInBits(Indices[Depth]);
+      } else if (IndexedTy->isArrayTy()) {
+        Type *ElementTy = IndexedTy->getSequentialElementType();
+        FieldOffset += DL.getTypeAllocSizeInBits(ElementTy) * Indices[Depth];
+      } else {
+        llvm_unreachable("Not a valid type to index into with extractvalue");
+      }
+    }
+
+    MetadataAsValue *AggMD = wrapMD(Agg);
+    for (auto *DII : DbgUsers) {
+      if (FieldOffset) {
+        applyOps(DII, {dwarf::DW_OP_constu, FieldOffset, dwarf::DW_OP_shr});
+      } else {
+        DII->setOperand(0, AggMD);
+        DEBUG(dbgs() << "SALVAGE:  " << *DII << '\n');
+      }
+    }
   }
 }
 
 unsigned llvm::removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB) {
   unsigned NumDeadInst = 0;
   // Delete the instructions backwards, as it has a reduced likelihood of
   // having to update as many def-use and use-def chains.
   Instruction *EndInst = BB->getTerminator(); // Last not to be deleted.

test/Transforms/InstCombine/debuginfo-variables.ll

 }
 
 define void @test_and(i64 %A) {
 ; CHECK-LABEL: @test_and(
 ; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %A, metadata !72, metadata !DIExpression(DW_OP_constu, 256, DW_OP_and, DW_OP_stack_value)), !dbg !73
   %1 = and i64 %A, 256
   ret void
 }
+
+define void @test_extractvalue_1([1 x i64] %Arr) {
+; CHECK-LABEL: @test_extractvalue_1(
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata [1 x i64] %Arr, metadata !77, metadata !DIExpression()), !dbg !78
+  %1 = extractvalue [1 x i64] %Arr, 0
+  ret void
+}
+
+define void @test_extractvalue_2([2 x i64] %Arr) {
+; CHECK-LABEL: @test_extractvalue_2(
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata [2 x i64] %Arr, metadata !82, metadata !DIExpression(DW_OP_constu, 64, DW_OP_shr, DW_OP_stack_value)), !dbg !83
+  %1 = extractvalue [2 x i64] %Arr, 1
+  ret void
+}
+
+%ty1 = type { i32 }
+
+define void @test_extractvalue_3(%ty1 %S) {
+; CHECK-LABEL: @test_extractvalue_3(
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty1 %S, metadata !87, metadata !DIExpression()), !dbg !88
+  %1 = extractvalue %ty1 %S, 0
+  ret void
+}
+
+; Note: this is an unpacked struct.
+%ty2 = type { [2 x i8], [2 x i32], %ty1, [2 x %ty1], <2 x i8>, [1 x [2 x i8]] }
+
+define void @test_extractvalue_4(%ty2 %S) {
+; CHECK-LABEL: @test_extractvalue_4(
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty2 %S, metadata !92, metadata !DIExpression(DW_OP_constu, 8, DW_OP_shr, DW_OP_stack_value)), !dbg !100
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty2 %S, metadata !94, metadata !DIExpression(DW_OP_constu, 64, DW_OP_shr, DW_OP_stack_value)), !dbg !101
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty2 %S, metadata !95, metadata !DIExpression(DW_OP_constu, 96, DW_OP_shr, DW_OP_stack_value)), !dbg !102
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty2 %S, metadata !96, metadata !DIExpression(DW_OP_constu, 160, DW_OP_shr, DW_OP_stack_value)), !dbg !103
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty2 %S, metadata !97, metadata !DIExpression(DW_OP_constu, 192, DW_OP_shr, DW_OP_stack_value)), !dbg !104
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty2 %S, metadata !99, metadata !DIExpression(DW_OP_constu, 216, DW_OP_shr, DW_OP_stack_value)), !dbg !105
+  %1 = extractvalue %ty2 %S, 0, 1
+  %2 = extractvalue %ty2 %S, 1, 1
+  %3 = extractvalue %ty2 %S, 2, 0
+  %4 = extractvalue %ty2 %S, 3, 1, 0
+  %5 = extractvalue %ty2 %S, 4
+  %6 = extractvalue %ty2 %S, 5, 0, 1
+  ret void
+}
+
+; Note: this is a packed struct.
+%ty3 = type <{ [2 x i8], [2 x i32], %ty1, [2 x %ty1], <2 x i8>, [1 x [2 x i8]] }>
+
+define void @test_extractvalue_5(%ty3 %S) {
+; CHECK-LABEL: @test_extractvalue_5(
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty3 %S, metadata !109, metadata !DIExpression(DW_OP_constu, 8, DW_OP_shr, DW_OP_stack_value)), !dbg !115
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty3 %S, metadata !110, metadata !DIExpression(DW_OP_constu, 48, DW_OP_shr, DW_OP_stack_value)), !dbg !116
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty3 %S, metadata !111, metadata !DIExpression(DW_OP_constu, 80, DW_OP_shr, DW_OP_stack_value)), !dbg !117
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty3 %S, metadata !112, metadata !DIExpression(DW_OP_constu, 144, DW_OP_shr, DW_OP_stack_value)), !dbg !118
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty3 %S, metadata !113, metadata !DIExpression(DW_OP_constu, 176, DW_OP_shr, DW_OP_stack_value)), !dbg !119
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata %ty3 %S, metadata !114, metadata !DIExpression(DW_OP_constu, 200, DW_OP_shr, DW_OP_stack_value)), !dbg !120
+  %1 = extractvalue %ty3 %S, 0, 1
+  %2 = extractvalue %ty3 %S, 1, 1
+  %3 = extractvalue %ty3 %S, 2, 0
+  %4 = extractvalue %ty3 %S, 3, 1, 0
+  %5 = extractvalue %ty3 %S, 4
+  %6 = extractvalue %ty3 %S, 5, 0, 1
+  ret void
+}