[PATCH] D110332: [DebugInfo] Limit the size of DIExpressions that we will salvage up to

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer created this revision.
StephenTozer added reviewers: spatel, dblaikie, aprantl, jmorse, probinson.
StephenTozer added a project: debug-info.
Herald added a subscriber: hiraditya.
StephenTozer requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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

This patch places an arbitrary limit on the size of DIExpressions that we will produce via salvaging, for performance reasons. This helps to fix a performance issue observed in the bug above, in which debug values would be salvaged hundreds of times, producing expressions with over 1000 elements and causing the compiler to hang. Limiting the size of debug values that we will produce to 128 largely fixes this issue.

It should be noted that this is not the only issue related to the bug above; InstCombine is also quadratically growing the number of debug intrinsics with each block it moves through, resulting in the number of intrinsics growing from 4561 to 70710 despite the fact that not a single one of the new intrinsics adds useful debug info. There are potential fixes for this, but I believe the problem of unlimited salvaging will still be an issue even if we smarten up InstCombine, as we can still legitimately salvage thousands of intrinsics thousands of times each before we determine that the resulting intrinsics provide no useful info. As such, I think this patch is probably a necessity that we can aim to reduce our dependence on with further changes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110332

Files:
  llvm/lib/Transforms/Utils/Local.cpp


Index: llvm/lib/Transforms/Utils/Local.cpp
===================================================================
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1733,9 +1733,11 @@
 
 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.
+  // These are arbitrary chosen limits on the maximum number of values and the
+  // maximum size of a debug expression we can salvage up to, used for
+  // performance reasons.
   const unsigned MaxDebugArgs = 16;
+  const unsigned MaxExpressionSize = 128;
   bool Salvaged = false;
 
   for (auto *DII : DbgUsers) {
@@ -1772,9 +1774,10 @@
       break;
 
     DII->replaceVariableLocationOp(&I, Op0);
-    if (AdditionalValues.empty()) {
+    bool IsValidSalvageExpr = SalvagedExpr->getNumElements() <= MaxExpressionSize;
+    if (AdditionalValues.empty() && IsValidSalvageExpr) {
       DII->setExpression(SalvagedExpr);
-    } else if (isa<DbgValueInst>(DII) &&
+    } else if (isa<DbgValueInst>(DII) && IsValidSalvageExpr &&
                DII->getNumVariableLocationOps() + AdditionalValues.size() <=
                    MaxDebugArgs) {
       DII->addVariableLocationOps(AdditionalValues, SalvagedExpr);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D110332.374531.patch
Type: text/x-patch
Size: 1407 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210923/c8d06568/attachment.bin>