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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1605
+    unsigned FieldOffset = 0;
+    for (unsigned I = 0, E = FieldIndices.size(); I < E; ++I) {
+      Type *IndexedTy = ExtractValueInst::getIndexedType(
----------------
aprantl wrote:
> aprantl wrote:
> > aprantl wrote:
> > > Why does this loop go all the way to FieldIndices.size()?
> > This is overloading `I` which makes the code harder to read.
> got it.. this is iterating over the field indices of the extract_value instruction, not over the data type! I was confused by this.
I'll add a comment.


================
Comment at: test/Transforms/InstCombine/debuginfo-variables.ll:125
+; 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
----------------
aprantl wrote:
> Hard-coding the metadata numbering (metadata !92, !dbg !100) seems to be really fragile, and probably doesn't add anything to the test.
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?


https://reviews.llvm.org/D43324