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

Vedant Kumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 16 19:04:44 PST 2018


vsk added inline comments.


================
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:
> vsk wrote:
> > 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?
> 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.
Gotcha, I'll fix this in the next update.


https://reviews.llvm.org/D43324





More information about the llvm-commits mailing list