[PATCH] D61184: [Salvage] Change salvage debug info implementation to use new DW_OP_LLVM_convert where needed

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1142
       default: break;
+      case dwarf::DW_OP_shr:
       case dwarf::DW_OP_plus:
----------------
StephenTozer wrote:
> djtodoro wrote:
> > aprantl wrote:
> > > Should this be a separate commit? This seems unrelated to the SDag change?
> > I think as well. This needs to be different patch.
> I'm currently investigating whether this is necessary for the bug fix or not - in this case, immediately prior to CodeGen we have the variable i64 %j, which represents two source variables:
> 
> ```
>   define dso_local i32 @h(i64 %j) local_unnamed_addr #0 !dbg !8 {
>   entry:
>     call void @llvm.dbg.value(metadata i64 %j, metadata !14, metadata !DIExpression()), !dbg !29
>     call void @llvm.dbg.value(metadata i64 %j, metadata !15, metadata !DIExpression(DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 32)), !dbg !29
>     call void @llvm.dbg.value(metadata i64 %j, metadata !15, metadata !DIExpression(DW_OP_constu, 32, DW_OP_shr, DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value, DW_OP_LLVM_fragment, 32, 32)), !dbg !29
> ```
> 
> At the ISel stage, the target architecture requires the i64 variable to be split across two 32 bit registers. We now have two 32 bit registers (one "low", one "high") which map exactly to two 32 bit source variables, but due to the previous transforms the finalized MIR looks like:
> ```    %1:gpr = COPY $r1
>     DBG_VALUE %1, $noreg, !14, !DIExpression(DW_OP_LLVM_fragment, 32, 32), debug-location !29
>     %0:gpr = COPY $r0
>     DBG_VALUE %0, $noreg, !14, !DIExpression(DW_OP_LLVM_fragment, 0, 32), debug-location !29
>     DBG_VALUE %0, $noreg, !15, !DIExpression(DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 32), debug-location !29
>     DBG_VALUE %0, $noreg, !15, !DIExpression(DW_OP_constu, 32, DW_OP_shr, DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value, DW_OP_LLVM_fragment, 32, 32), debug-location !29
> ```
> 
> As you can see the 32 bit DW_OP_shr is now being applied to the low 32 bit register in order to get the value in the high register, which won't work. Any operation which expects any kind of "carry over" will fail in the same way when a DBG_VALUE is split. I'm currently looking into whether there's a better way to specifically handle shift operations, since adding DW_OP_shr here still results in a variable being set as "undef" when it could potentially be recoverable.
I'm not opposed to supporting shr here, I just think it should be a separately tested patch that can land in preparation of this one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61184/new/

https://reviews.llvm.org/D61184