Add support for salvaging debug info from icmp instrcuctions.

[rastogishubham]

Summary

salvageDebugInfo() is a function that allows us to reatin debug info for instructions that have been optimized out. Currently, it doesn't support salvaging the debug information from icmp instrcutions, but DWARF expressions can emulate an icmp by using the DWARF conditional expressions. This patch adds support for salvaging debug information from icmp instructions.

[bulbazord]

llvm/lib/Transforms/Utils/Local.cpp

2056

nit: you can drop the check for ConstInt in this condition because it must be valid due to the above check.

2064

nit: You could push_back(ConstInt) since it's the same thing you already computed instead of calling Icmp->getOperand(1) again.

2068–2070

You've already changed AdditionalValues by the time you get to this point... Do the callers verify that the return value of this function is nullptr before using AdditionalValues? Even if they do today, somebody may use this function without realizing that this is the behavior with surprising results. My suggestion would be to only mutate the output references (Opcodes and AdditionalValues) when you know that this function will succeed.

[rastogishubham]

Removed unnecessary check for ConstInt, removed redundant call to getOperand(1) and moved check for predicate above mutating AdditionalValues

llvm/lib/Transforms/Utils/Local.cpp

2068–2070

That is a good catch, the check for the predicate doesn't have to be done at the end, so I can move it above the mutation of AdditionalValues

[aprantl]

llvm/lib/Transforms/Utils/Local.cpp

1996

I guess the assumption here is that in the typed stack the signedness of the operation is implicit? Might be good to document that here.

llvm/test/DebugInfo/salvage-icmp.ll

15

Can you add tests for the other operations, too?

[rastogishubham]

Updated test to check all operations, changed output to emit a DW_OP_constu or DW_OP_consts instead of a DW_OP_llvm_arg for constant values

[StephenTozer]

A few comments and questions inline - glad to see more operators being added to the salvageable list!

llvm/lib/Transforms/Utils/Local.cpp

2049–2051

Is there a reason for the IntType template here? At the moment we wouldn't expect any type other than uint64_t to appear here, as it's the width of a DWARF expression operand.

2055–2057

I don't think this early-exit to prevent a large DIExpression should be necessary - the only functions that call into salvageDebugInfoImpl either don't accept any additional values, or have a limit on the size of the DIExpression they will produce.

2062–2066

I think this is the cause of the (as far as I can tell) erroneous DW_OP_LLVM_args in the expression in the test; these steps should only happen if you're salvaging a variadic expression, i.e. if Icmp->getOperand(1) is an Instruction rather than a ConstantInt - it should be fine if you just delete these lines outright, unless you change the function to also support salvaging a pair of Instructions.

To summarize the intention of this code, DW_OP_LLVM_arg should only appear in variadic expressions as a way to represent the list of SSA values being passed to the expression. The first four lines check whether the expression is already variadic and converts it to be variadic if not, and then the last line adds the LLVM_arg for the new SSA value (which in this case does not exist), while the SSA value is added to AdditionalValues. The existing functions are not commented or documented very well, my apologies!

llvm/test/DebugInfo/salvage-icmp.ll

7–9

Does this test pass as written? I see references to various values of DW_OP_LLVM_arg in the expression, but the !DIArgList contains 1 element - I'd expect this to trigger an assertion.

[rastogishubham]

Added support for non-constant values, fixed bug with DW_OP_llvm_arg, added a new testcase to check for non-constant values

llvm/lib/Transforms/Utils/Local.cpp

2049–2051

I am sorry about that, I was working on a patch where I was going to pass an int64_t for signed comparisons, I ended up realizing that a DW_OP_consts can be used with a uint64_t just fine, so I removed the template everywhere, but I guess I missed a place

2062–2066

Thanks for explaining the logic to me, I did get a bit confused there

[rastogishubham]

NFC change, moved common code in handling binary instructions and icmp instructions with both operands as ssa values into its own function

[aprantl]

Thanks, I think all of my concerns have been addressed!
Does lib/CodeGen/AsmPrinter/DwarfExpression.cpp need to be updated for the new opcodes or will just work?

[StephenTozer]

Thanks for the update, changes look good (with some minor comments inline)!

In D150216#4342620, @aprantl wrote:

Thanks, I think all of my concerns have been addressed!
Does lib/CodeGen/AsmPrinter/DwarfExpression.cpp need to be updated for the new opcodes or will just work?

That's a good point - currently these ops will be unhandled and trigger an assert in DwarfExpression::addExpression; after fixing that up, it'd probably be a good idea to add one or more extra tests to exercise ISel and DWARF emission. You could model the tests off of llvm\test\DebugInfo\X86\debug_value_list_selectiondag.ll and llvm\test\DebugInfo\X86\dbg_value_list_emission.mir, or add additional debug values to existing tests to cover them.

llvm/lib/Transforms/Utils/Local.cpp

2036–2037

Very small point, but the coding standards recommend braces to be used for else if the corresponding if is using them.

2089–2090

Ditto the above comment.

[rastogishubham]

Added braces for the else, since the if also has them

[rastogishubham]

Updated DWARFExpression.cpp to support the conditional operators, updated tests to check for them as well

[rastogishubham]

Added new line at the end of debug_value_list_selectiondag.ll

[jmorse]

This sounds like a good direction to take; a comment inline about whether the patch as-is only salvages icmps with one constant operand (it appears to salvage more?).

It might be worth relaxing that condition and allowing all icmps to be salvaged, if it doesn't significantly increase file size on some large code samples.

llvm/lib/Transforms/Utils/Local.cpp

1985

As this is function local, can it be static too?

2075–2076

Possibly I've missed something here, but this statement (only salvage icmps with a constant integer operand) doesn't seem to be true, as the second dbg.value in the test you're adding salvages many icmps with Value operands:

%icmp18 = icmp slt i32 %icmp17.1, %b, !dbg !15
%icmp19 = icmp ule i32 %icmp18.1, %a, !dbg !15
%icmp20 = icmp sle i32 %icmp19.1, %a, !dbg !15

and the corresponding CHECK line has a lot Value inputs in the DIArgList.

[rastogishubham]

Updated wrong comment about only handling icmps with one Integer operand, and made handleSSAValueOperands static

llvm/lib/IR/DebugInfoMetadata.cpp

     case dwarf::DW_OP_lit0:
     case dwarf::DW_OP_not:
     case dwarf::DW_OP_dup:
     case dwarf::DW_OP_regx:
     case dwarf::DW_OP_bregx:
     case dwarf::DW_OP_push_object_address:
     case dwarf::DW_OP_over:
     case dwarf::DW_OP_consts:
+    case dwarf::DW_OP_eq:
+    case dwarf::DW_OP_ne:
+    case dwarf::DW_OP_gt:
+    case dwarf::DW_OP_ge:
+    case dwarf::DW_OP_lt:
+    case dwarf::DW_OP_le:
       break;
     }
   }
   return true;
 }
 
 bool DIExpression::isImplicit() const {
   if (!isValid())

llvm/lib/Transforms/Utils/Local.cpp

   case Instruction::AShr:
     return dwarf::DW_OP_shra;
   default:
     // TODO: Salvage from each kind of binop we know about.
     return 0;
   }
 }
 
+uint64_t getDwarfOpForIcmpPred(CmpInst::Predicate Pred) {
+  switch (Pred) {
+  case CmpInst::ICMP_EQ:
+    return dwarf::DW_OP_eq;
+  case CmpInst::ICMP_NE:
+    return dwarf::DW_OP_ne;
+  case CmpInst::ICMP_UGT:
+  case CmpInst::ICMP_SGT:
+    return dwarf::DW_OP_gt;
+  case CmpInst::ICMP_UGE:
+  case CmpInst::ICMP_SGE:
+    return dwarf::DW_OP_ge;
+  case CmpInst::ICMP_ULT:
+  case CmpInst::ICMP_SLT:
+    return dwarf::DW_OP_lt;
+  case CmpInst::ICMP_ULE:
+  case CmpInst::ICMP_SLE:
+    return dwarf::DW_OP_le;
+  default:
+    return 0;
+  }
+}
+
 Value *getSalvageOpsForBinOp(BinaryOperator *BI, uint64_t CurrentLocOps,
                              SmallVectorImpl<uint64_t> &Opcodes,
                              SmallVectorImpl<Value *> &AdditionalValues) {
   // Handle binary operations with constant integer operands as a special case.
   auto *ConstInt = dyn_cast<ConstantInt>(BI->getOperand(1));
   // Values wider than 64 bits cannot be represented within a DIExpression.
   if (ConstInt && ConstInt->getBitWidth() > 64)
     return nullptr;

     Opcodes.append({dwarf::DW_OP_constu, Val});
   } else {
     if (!CurrentLocOps) {
       Opcodes.append({dwarf::DW_OP_LLVM_arg, 0});
       CurrentLocOps = 1;
     }
     Opcodes.append({dwarf::DW_OP_LLVM_arg, CurrentLocOps});
     AdditionalValues.push_back(BI->getOperand(1));
   }
 
   // Add salvaged binary operator to expression stack, if it has a valid
   // representation in a DIExpression.
   uint64_t DwarfBinOp = getDwarfOpForBinOp(BinOpcode);
   if (!DwarfBinOp)
     return nullptr;
   Opcodes.push_back(DwarfBinOp);
   return BI->getOperand(0);
 }
 
+Value *getSalvageOpsForIcmpOp(ICmpInst *Icmp, uint64_t CurrentLocOps,
+                              SmallVectorImpl<uint64_t> &Opcodes,
+                              SmallVectorImpl<Value *> &AdditionalValues) {
+  // Only salvage debug info for IcmpInst that have at least one constant
+  // integer operand, to prevent the explosion of the DIExpression.
+  auto *ConstInt = dyn_cast<ConstantInt>(Icmp->getOperand(1));
+  if (!ConstInt)
+    return nullptr;
+  // Values wider than 64 bits cannot be represented within a DIExpression.
+  if (ConstInt->getBitWidth() > 64)
+    return nullptr;
+
+  // Add salvaged binary operator to expression stack, if it has a valid
+  // representation in a DIExpression.
+  uint64_t DwarfIcmpOp = getDwarfOpForIcmpPred(Icmp->getPredicate());
+  if (!DwarfIcmpOp)
+    return nullptr;
+
+  if (!CurrentLocOps) {
+    Opcodes.append({dwarf::DW_OP_LLVM_arg, 0});
+    CurrentLocOps = 1;
+  }
+  Opcodes.append({dwarf::DW_OP_LLVM_arg, CurrentLocOps});
+  AdditionalValues.push_back(ConstInt);
+
+  Opcodes.push_back(DwarfIcmpOp);
+  return Icmp->getOperand(0);
+}
+
 Value *llvm::salvageDebugInfoImpl(Instruction &I, uint64_t CurrentLocOps,
                                   SmallVectorImpl<uint64_t> &Ops,
                                   SmallVectorImpl<Value *> &AdditionalValues) {
   auto &M = *I.getModule();
   auto &DL = M.getDataLayout();
 
   if (auto *CI = dyn_cast<CastInst>(&I)) {
     Value *FromValue = CI->getOperand(0);
     // No-op casts are irrelevant for debug info.
     if (CI->isNoopCast(DL)) {
       return FromValue;
     }
 
     Type *Type = CI->getType();
     if (Type->isPointerTy())
       Type = DL.getIntPtrType(Type);
     // Casts other than Trunc, SExt, or ZExt to scalar types cannot be salvaged.
     if (Type->isVectorTy() ||
         !(isa<TruncInst>(&I) || isa<SExtInst>(&I) || isa<ZExtInst>(&I) ||
           isa<IntToPtrInst>(&I) || isa<PtrToIntInst>(&I)))
       return nullptr;
 
     llvm::Type *FromType = FromValue->getType();
     if (FromType->isPointerTy())
       FromType = DL.getIntPtrType(FromType);
 
     unsigned FromTypeBitSize = FromType->getScalarSizeInBits();
     unsigned ToTypeBitSize = Type->getScalarSizeInBits();
 
     auto ExtOps = DIExpression::getExtOps(FromTypeBitSize, ToTypeBitSize,
                                           isa<SExtInst>(&I));
     Ops.append(ExtOps.begin(), ExtOps.end());
     return FromValue;
   }
 
   if (auto *GEP = dyn_cast<GetElementPtrInst>(&I))
     return getSalvageOpsForGEP(GEP, DL, CurrentLocOps, Ops, AdditionalValues);
   if (auto *BI = dyn_cast<BinaryOperator>(&I))
     return getSalvageOpsForBinOp(BI, CurrentLocOps, Ops, AdditionalValues);
+  if (auto *IC = dyn_cast<ICmpInst>(&I))
+    return getSalvageOpsForIcmpOp(IC, CurrentLocOps, Ops, AdditionalValues);
 
   // *Not* to do: we should not attempt to salvage load instructions,
   // because the validity and lifetime of a dbg.value containing
   // DW_OP_deref becomes difficult to analyze. See PR40628 for examples.
   return nullptr;
 }
 
 /// A replacement for a dbg.value expression.

llvm/test/DebugInfo/salvage-icmp.ll

+; RUN: opt %s -passes=dce -S | FileCheck %s
+
+; Tests the results of salvaging variadic dbg.values that use the same SSA value
+; multiple times.
+
+; CHECK: call void @llvm.dbg.value(metadata !DIArgList(i32 %a, i32 0),
+; CHECK-SAME: ![[VAR_C:[0-9]+]],
+; CHECK-SAME: !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_ne, DW_OP_stack_value))
+
+; CHECK: ![[VAR_C]] = !DILocalVariable(name: "c"
+
+define i32 @"?multiply@@YAHHH@Z"(i32 %a, i32 %b) !dbg !8 {
+entry:
+  %icmp1 = icmp ne i32 %a, 0, !dbg !15
+  call void @llvm.dbg.value(metadata !DIArgList(i1 %icmp1), metadata !16, metadata !DIExpression()), !dbg !13
+  %mul = mul nsw i32 %a, %b, !dbg !17
+  ret i32 %mul, !dbg !17
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 11.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"CodeView", i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 11.0.0"}
+!8 = distinct !DISubprogram(name: "multiply", linkageName: "?multiply@@YAHHH@Z", scope: !1, file: !1, line: 1, type: !9, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !11, !11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !DILocalVariable(name: "b", arg: 2, scope: !8, file: !1, line: 1, type: !11)
+!13 = !DILocation(line: 0, scope: !8)
+!14 = !DILocalVariable(name: "a", arg: 1, scope: !8, file: !1, line: 1, type: !11)
+!15 = !DILocation(line: 2, scope: !8)
+!16 = !DILocalVariable(name: "c", scope: !8, file: !1, line: 2, type: !11)
+!17 = !DILocation(line: 3, scope: !8)