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

[StephenTozer]

Summary

Fixes issue: https://bugs.llvm.org/show_bug.cgi?id=40645

Previously, LLVM had no functional way of performing casts inside of a DIExpression(), which made salvaging cast instructions other than Noop casts impossible. With the recent addition of DW_OP_LLVM_convert this salvaging is now possible, and so can be used to fix the attached bug as well as any cases where SExt instruction results are lost in the debugging metadata. This patch introduces this fix by expanding the salvage debug info method to cover these cases using the new operator.

[dstenb]

llvm/lib/Transforms/Utils/Local.cpp

1677

There is a early bail-out for ZExtInst above:

if (CI->isNoopCast(DL) || isa<ZExtInst>(&I))
  return SrcDIExpr;

so that condition in this if statement appears to be dead code as it is now.

I assume that we no longer should bail out for zexts (?).

[StephenTozer]

As per the previous comment, ZExt is equivalent to a Noop on the DWARF expression stack, so can be caught by the same early exit.

[markus]

Have you verified that this results in the desired DW_OPs being emitted in the end and that the (or a) debugger makes sense out of it?

Since the expression stack is a bit sloppy about types it defaults to the generic type (size of address on the target machine), so without having previously set the type to something else a single DW_OP_LLVM_convert does not mean much. This is the reason that llvm::replaceAllDbgUsesWith in lib/Transforms/Utils/Local.cpp inserts a pair (the first one is simply used to set the type of the current stack element and the second one performs the actual conversion).

For now inserting a pair would probably be the easiest way forward. A bit prettier would be to introduce a DW_OP_LLVM_reinterpret op to do that job. Longer term it would be helpful if all values had a specified type immediately when they are put on the stack.

[dstenb]

llvm/lib/Transforms/Utils/Local.cpp

1677

Also, perhaps this should be inverted to an early exit instead?

[markus]

In D61184#1480359, @markus wrote:

A bit prettier would be to introduce a DW_OP_LLVM_reinterpret op to do that job.

I am preparing a patch for that right now. Regardless of this review it should make the existing DW_OP_LLVM_convert stuff a bit cleaner.

[markus]

In D61184#1481992, @markus wrote:

In D61184#1480359, @markus wrote:

A bit prettier would be to introduce a DW_OP_LLVM_reinterpret op to do that job.

I am preparing a patch for that right now. Regardless of this review it should make the existing DW_OP_LLVM_convert stuff a bit cleaner.

Actually forget all I said about DW_OP_reinterpret, it turns out that I did not read the spec properly and did not pay attention to the last sentence saying that: The type of the operand and result type must have the same size in bits.

So clearly it cannot be used like I suggested.

What I said about using two DW_OP_LLVM_convert in sequence still holds though.

[StephenTozer]

Updated to use convert OP correctly as suggested by markus.

[aprantl]

llvm/lib/Transforms/Utils/Local.cpp

1691

clang-format?

[StephenTozer]

Remove incorrectly included lines.

[markus]

The DW_OP_LLVM_convert stuff looks-good-to-me but I don't feel that I have authority to approve for the rest so please don't wait for any input from my part.

[probinson]

llvm/lib/Transforms/Utils/Local.cpp

1691

Looks to me like this 'if' does more than check for vectors?

[StephenTozer]

Update comment

[probinson]

LGTM after you add the full-stop noted below.

llvm/lib/Transforms/Utils/Local.cpp

1691

Needs a full-stop at the end of the sentence.

[mstorsjo]

This broke compilation of libtiff for arm, see https://bugs.llvm.org/show_bug.cgi?id=41931 for details.

[djtodoro]

Thanks for working on this, it seems very useful! Any updates?

[StephenTozer]

Reopening, as the merged patch caused a regression issue and needed to be reverted; this update should fix said regression issue.

[aprantl]

What was the fix for the regression and is it also tested?

[StephenTozer]

The regression issue is documented in the bug above: https://bugs.llvm.org/show_bug.cgi?id=41931
The main issue can be summarised briefly as "splitting a fragment DIExpression across multiple registers is currently broken", which is what I've submitted a fix for (see comments in SelectionDAGBuilder.cpp); the test for the new behaviour is in fragmented-args-multiple-regs.ll

[djtodoro]

Thanks for this (a few comments added).

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5520 ↗

(On Diff #215625)

I guess this is unwanted.

llvm/test/CodeGen/ARM/fragmented-args-multiple-regs.ll

34 ↗

(On Diff #215625)

We can delete this.

35 ↗

(On Diff #215625)

And this as well.

41 ↗

(On Diff #215625)

We can also update the version to 10.0.0.

48 ↗

(On Diff #215625)

Here as well.

llvm/test/DebugInfo/salvage-cast-debug-info.ll

22

Here as well.

[djtodoro]

Looks like this was accepted (with minor nitpicks) long time ago. Please let us know if you have any obstacle to commit it.

[aprantl]

llvm/lib/IR/DebugInfoMetadata.cpp

1142 ↗

(On Diff #215625)

Should this be a separate commit? This seems unrelated to the SDag change?

llvm/test/CodeGen/ARM/fragmented-args-multiple-regs.ll

35 ↗

(On Diff #215625)

Still needs to happen.

[djtodoro]

llvm/lib/IR/DebugInfoMetadata.cpp

1142 ↗

(On Diff #215625)

I think as well. This needs to be different patch.

[StephenTozer]

llvm/lib/IR/DebugInfoMetadata.cpp

1142 ↗

(On Diff #215625)

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.

[aprantl]

llvm/lib/IR/DebugInfoMetadata.cpp

1142 ↗

(On Diff #215625)

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.

[djtodoro]

I think this one is almost (with the comments addressed) ready to go, but I think that the D70248 is preparation for this one? Should we create a stack of patches for those two patches?

If D70498 is landed, please make sure to use the API here.

[StephenTozer]

Rebased and updated for latest master, fixed clang version and unused attributes in tests.

[vsk]

This seems to be in good shape overall and the SelectionDAG changes look reasonable. I don't intend my comments here to block committing.

llvm/test/DebugInfo/X86/dbg-value-dropped-instcombine.ll

26 ↗

(On Diff #233641)

This looks like it would have matched before this patch. Consider ; CHECK: @llvm.dbg.value(metadata i32 %{{.*}},?

llvm/test/Transforms/InstCombine/pr43893.ll

16 ↗

(On Diff #233641)

I'm not sure I understand this change. Why is the dbg.value for %conv deleted?

[djtodoro]

Please address my comments and try using the DIExpression::appendExt(). Thanks.

[djtodoro]

llvm/lib/Transforms/Utils/Local.cpp

1705

More precisely, the DIExpression::appendExt() should be used here.

[StephenTozer]

llvm/test/Transforms/InstCombine/pr43893.ll

16 ↗

(On Diff #233641)

The purpose of this test as stated in the comment at the top was to check that, when the trivially deletable %conv was deleted, metadata that depended on it was set to undef. Reading that comment it seemed that the important component of the test wasn't the existence of the dbg.value itself, but confirming that it would be set undef rather than being given an incorrect value. As of this patch however, %conv can be salvaged, which seems to defeat the point of the test. As %udiv cannot be salvaged, it seemed like an appropriate stand-in.

[StephenTozer]

Use shared functionality for zero/sign extension ops, fix tests

[vsk]

llvm/test/Transforms/InstCombine/pr43893.ll

16 ↗

(On Diff #233641)

Ah, got it, sgtm.

[StephenTozer]

@djtodoro Does this latest change look good to you?

Since the only other outstanding comments were not intended to block committing, if this is considered resolved and there are no further comments I'll consider this implicitly LGTM'd; although an explicit response as of the latest changes would be preferred.

[djtodoro]

LGTM, thanks!

[djtodoro]

Sorry, one more nit included.

llvm/include/llvm/IR/DebugInfoMetadata.h

2555 ↗

(On Diff #233788)

Have you considered using the using for declaration of the std::array<uint64_t, 6 return type?

[StephenTozer]

Format, addressing review comments

[djtodoro]

Looks good to me! Thanks!

[StephenTozer]

Merged in commit: llvmorg-10-init-12272-g89d19d60adb9

llvm/lib/Transforms/Utils/Local.cpp

auto applyOps =
     SmallVector<uint64_t, 8> Ops(Opcodes);
     return doSalvage(Ops);
   };
 
   if (auto *CI = dyn_cast<CastInst>(&I)) {
     // No-op casts and zexts are irrelevant for debug info.
     if (CI->isNoopCast(DL) || isa<ZExtInst>(&I))
       return SrcDIExpr;
+    Type *Type = CI->getType();
+    // There does not appear to be any op for vector casting.
+    if (Type->isVectorTy())
+      return nullptr;
+
+    if (isa<TruncInst>(&I) || isa<ZExtInst>(&I) || isa<SExtInst>(&I)) {
+      unsigned TypeBitSize = Type->getScalarSizeInBits();
+
+      // The result of the cast will be sign extended iff the instruction is a
+      // SExt; signedness is otherwise irrelevant on the expression stack.
+      unsigned Encoding =
+          isa<SExtInst>(&I) ? dwarf::DW_ATE_signed : dwarf::DW_ATE_unsigned;
+
+      return applyOps({dwarf::DW_OP_LLVM_convert, TypeBitSize, Encoding});
+    }
+
     return nullptr;
   } else if (auto *GEP = dyn_cast<GetElementPtrInst>(&I)) {
     unsigned BitWidth =
         M.getDataLayout().getIndexSizeInBits(GEP->getPointerAddressSpace());
     // Rewrite a constant GEP into a DIExpression.
     APInt Offset(BitWidth, 0);
     if (GEP->accumulateConstantOffset(M.getDataLayout(), Offset)) {
       return applyOffset(Offset.getSExtValue());
     } else {
       return nullptr;
     }
   } else if (auto *BI = dyn_cast<BinaryOperator>(&I)) {
     // Rewrite binary operations with constant integer operands.
     auto *ConstInt = dyn_cast<ConstantInt>(I.getOperand(1));
     if (!ConstInt || ConstInt->getBitWidth() > 64)
       return nullptr;
 
     uint64_t Val = ConstInt->getSExtValue();
     switch (BI->getOpcode()) {
     case Instruction::Add:
       return applyOffset(Val);
     case Instruction::Sub:
       return applyOffset(-int64_t(Val));
     case Instruction::Mul:
       return applyOps({dwarf::DW_OP_constu, Val, dwarf::DW_OP_mul});
     case Instruction::SDiv:

llvm/test/DebugInfo/salvage-cast-debug-info.ll

+; RUN: opt %s -debugify -early-cse -S | FileCheck %s
+define i32 @foo(i64 %nose, i32 %more) {
+; CHECK-LABEL: @foo(
+; CHECK: call void @llvm.dbg.value(metadata i64 %nose, metadata [[V1:![0-9]+]], metadata !DIExpression(DW_OP_LLVM_convert, 32
+; CHECK: call void @llvm.dbg.value(metadata i64 %nose.shift, metadata [[V2:![0-9]+]]
+; CHECK: call void @llvm.dbg.value(metadata i64 %nose.shift, metadata [[V3:![0-9]+]], metadata !DIExpression(DW_OP_LLVM_convert, 32
+
+entry:
+  %nose.trunc = trunc i64 %nose to i32
+  %nose.shift = lshr i64 %nose, 32
+  %nose.trunc.2 = trunc i64 %nose.shift to i32
+  %add = add nsw i32 %more, 1
+  ret i32 %add
+}
+
+!llvm.module.flags = !{!0, !1}
+!llvm.ident = !{!2}
+
+!0 = !{i32 1, !"wchar_size", i32 2}
+!1 = !{i32 7, !"PIC Level", i32 2}
+!2 = !{!"clang version 9.0.0 "}
+
+; CHECK: [[V1]] = !DILocalVariable(
+; CHECK: [[V2]] = !DILocalVariable(
+; CHECK: [[V3]] = !DILocalVariable(

llvm/test/Transforms/InstCombine/cast-mul-select.ll

 ; RUN: opt < %s -instcombine -S | FileCheck %s
 ; RUN: opt -debugify -instcombine -S < %s | FileCheck %s -check-prefix DBGINFO
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32"
 
 define i32 @mul(i32 %x, i32 %y) {
 ; CHECK-LABEL: @mul(
 ; CHECK-NEXT:    [[C:%.*]] = mul i32 [[X:%.*]], [[Y:%.*]]
 ; CHECK-NEXT:    [[D:%.*]] = and i32 [[C]], 255
 ; CHECK-NEXT:    ret i32 [[D]]
 
 ; Test that when zext is evaluated in different type
 ; we preserve the debug information in the resulting
 ; instruction.
 ; DBGINFO-LABEL: @mul(
+; DBGINFO-NEXT:    call void @llvm.dbg.value(metadata i32 %x
+; DBGINFO-NEXT:    call void @llvm.dbg.value(metadata i32 %y
 ; DBGINFO-NEXT:    [[C:%.*]] = mul i32 {{.*}}
 ; DBGINFO-NEXT:    [[D:%.*]] = and i32 {{.*}}
 ; DBGINFO-NEXT:    call void @llvm.dbg.value(metadata i32 [[C]]
 ; DBGINFO-NEXT:    call void @llvm.dbg.value(metadata i32 [[D]]
 
   %A = trunc i32 %x to i8
   %B = trunc i32 %y to i8
   %C = mul i8 %A, %B