[DebugInfo] Limit the number of values that may be referenced by a dbg.value

[StephenTozer]

Summary

Following the merge of D91722, a case has been found where a large number of excessively large DIArgLists were created as a result of salvaging, effectively freezing the compiler. In order to prevent such extreme cases from affecting performance, this patch arbitrarily constrains DIArgLists to containing 16 arguments - any attempt to salvage them further will result in an undef dbg.value.

Although this patch is straightforward and should help deal with the prior performance issue, there is a further step that can be taken that may also help, which would be to reduce the size of undef dbg.values. Currently when we set a dbg.value undef, we just replace the lost value with an UndefValue of equal type, but we could further reduce the size of these dbg.values by removing the contents of the DIArgList and DIExpression, with the exception of any DIExpression operators that still have meaning (such as DW_OP_LLVM_fragment). I can make this into a separate patch, or fold it into this one - any thoughts?

[djtodoro]

This makes sense to me.
I remember that when we were experimenting with adding support for salvaging debug info for binary operations dealing with 2 SSA values, we needed to make something like this, since we had encountered some dramatic cases even when building clang itself (I've mentioned this on a patch from this area). Also, I don't have a strong opinion about whether it should be 16 or some other magic value.

[jmorse]

For the record, this LGTM too. Picking a better cut-off point is probably something we can experiment with in the future.

[wenlei]

Thanks for the change. I tried a few limits on the repro case I mentioned in D91722. It does look like 16 is a sweet spot at least for that case. :) Without forcing a limit, the case runs for 10+ hours..

MaxDebugArgs	time
4	20.5s
8	20.8s
16	20.7s
32	22.0s
64	31.2s

[wenlei]

lgtm, thanks.

llvm/lib/Transforms/Utils/Local.cpp

 void llvm::salvageDebugInfo(Instruction &I) {
   SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
   findDbgUsers(DbgUsers, &I);
   salvageDebugInfoForDbgValues(I, DbgUsers);
 }
 
 void llvm::salvageDebugInfoForDbgValues(
     Instruction &I, ArrayRef<DbgVariableIntrinsic *> DbgUsers) {
+  // This is an arbitrary chosen limit on the maximum number of values we can
+  // salvage up to in a DIArgList, used for performance reasons.
+  const unsigned MaxDebugArgs = 16;
   bool Salvaged = false;
 
   for (auto *DII : DbgUsers) {
     // Do not add DW_OP_stack_value for DbgDeclare and DbgAddr, because they
     // are implicitly pointing out the value as a DWARF memory location
     // description.
     bool StackValue = isa<DbgValueInst>(DII);
     auto DIILocation = DII->location_ops();
     assert(
         is_contained(DIILocation, &I) &&
         "DbgVariableIntrinsic must use salvaged instruction as its location");
     unsigned LocNo = std::distance(DIILocation.begin(), find(DIILocation, &I));
     SmallVector<Value *, 4> AdditionalValues;
     DIExpression *SalvagedExpr = salvageDebugInfoImpl(
         I, DII->getExpression(), StackValue, LocNo, AdditionalValues);
 
     // salvageDebugInfoImpl should fail on examining the first element of
     // DbgUsers, or none of them.
     if (!SalvagedExpr)
       break;
 
     DII->replaceVariableLocationOp(&I, I.getOperand(0));
     if (AdditionalValues.empty()) {
       DII->setExpression(SalvagedExpr);
-    } else if (isa<DbgValueInst>(DII)) {
+    } else if (isa<DbgValueInst>(DII) &&
+               DII->getNumVariableLocationOps() + AdditionalValues.size() <=
+                   MaxDebugArgs) {
       DII->addVariableLocationOps(AdditionalValues, SalvagedExpr);
     } else {
       // Do not salvage using DIArgList for dbg.addr/dbg.declare, as it is
       // currently only valid for stack value expressions.
+      // Also do not salvage if the resulting DIArgList would contain an
+      // unreasonably large number of values.
       Value *Undef = UndefValue::get(I.getOperand(0)->getType());
       DII->replaceVariableLocationOp(I.getOperand(0), Undef);
     }
     LLVM_DEBUG(dbgs() << "SALVAGE: " << *DII << '\n');
     Salvaged = true;
   }
 
   if (Salvaged)

llvm/test/DebugInfo/limit-arglist-size.ll

+; RUN: opt -S -instcombine %s -o - | FileCheck %s
+
+; For performance reasons, we currently limit the number of values that can be
+; referenced by a dbg.value to 16. This test checks that we do not exceed this
+; limit during salvaging.
+
+; CHECK: DIArgList(i32 undef
+; CHECK-NOT: DW_OP_LLVM_arg, 16
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @_Z3foov() local_unnamed_addr !dbg !9 {
+entry:
+  %call = call i32 @_Z3barv(), !dbg !14
+  %call1 = call i32 @_Z3barv(), !dbg !14
+  %add = add nsw i32 %call, %call1, !dbg !14
+  %call2 = call i32 @_Z3barv(), !dbg !14
+  %call4 = call i32 @_Z3barv(), !dbg !14
+  %call6 = call i32 @_Z3barv(), !dbg !14
+  %call8 = call i32 @_Z3barv(), !dbg !14
+  %call10 = call i32 @_Z3barv(), !dbg !14
+  %call12 = call i32 @_Z3barv(), !dbg !14
+  %call14 = call i32 @_Z3barv(), !dbg !14
+  %call16 = call i32 @_Z3barv(), !dbg !14
+  %call18 = call i32 @_Z3barv(), !dbg !14
+  %call20 = call i32 @_Z3barv(), !dbg !14
+  %call22 = call i32 @_Z3barv(), !dbg !14
+  %call24 = call i32 @_Z3barv(), !dbg !14
+  %call26 = call i32 @_Z3barv(), !dbg !14
+  %call28 = call i32 @_Z3barv(), !dbg !14
+  %call30 = call i32 @_Z3barv(), !dbg !14
+  call void @llvm.dbg.value(metadata !DIArgList(i32 %add, i32 %call30, i32 %call28, i32 %call26, i32 %call24, i32 %call22, i32 %call20, i32 %call18, i32 %call16, i32 %call14, i32 %call12, i32 %call10, i32 %call8, i32 %call6, i32 %call4, i32 %call2), metadata !15, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 15, DW_OP_plus, DW_OP_LLVM_arg, 14, DW_OP_plus, DW_OP_LLVM_arg, 13, DW_OP_plus, DW_OP_LLVM_arg, 12, DW_OP_plus, DW_OP_LLVM_arg, 11, DW_OP_plus, DW_OP_LLVM_arg, 10, DW_OP_plus, DW_OP_LLVM_arg, 9, DW_OP_plus, DW_OP_LLVM_arg, 8, DW_OP_plus, DW_OP_LLVM_arg, 7, DW_OP_plus, DW_OP_LLVM_arg, 6, DW_OP_plus, DW_OP_LLVM_arg, 5, DW_OP_plus, DW_OP_LLVM_arg, 4, DW_OP_plus, DW_OP_LLVM_arg, 3, DW_OP_plus, DW_OP_LLVM_arg, 2, DW_OP_plus, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_stack_value)), !dbg !16
+  %call32 = call i32 @_Z3barv(), !dbg !17
+  ret i32 %call32, !dbg !17
+}
+
+declare dso_local i32 @_Z3barv() local_unnamed_addr
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 13.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "limit-arglist-size.cpp", directory: "/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 7, !"uwtable", i32 1}
+!7 = !{i32 7, !"frame-pointer", i32 2}
+!8 = !{!"clang version 13.0.0"}
+!9 = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: !10, file: !10, line: 3, type: !11, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!10 = !DIFile(filename: "./limit-arglist-size.cpp", directory: "/")
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocation(line: 4, scope: !9)
+!15 = !DILocalVariable(name: "v16", scope: !9, file: !10, line: 4, type: !13)
+!16 = !DILocation(line: 0, scope: !9)
+!17 = !DILocation(line: 5, scope: !9)