[Debugify] Limit number of processed functions for original mode

[ntesic]

Summary

Debugify in OriginalDebugInfo mode, does (DebugInfo) collect-before-pass & check-after-pass
for each instruction, which is pretty expensive. When used to analyze DebugInfo losses
in large projects (like LLVM), this raises the build time unacceptably.
This patch introduces a limit for the number of processed functions per compile unit.
By default, the limit is set to UINT_MAX (practically unlimited), and by using the introduced
option -debugify-func-limit the limit could be set to any positive integer number.

[StephenTozer]

Performance seems like a serious issue when working with large projects here, but I have some questions/thoughts about this approach:

Is there any good reason to have an option to set the limit to zero? Unless I'm missing something, that would be equivalent to just not running debugify at all, which seems like a redundant option to have when debugify is an optional flag to begin with.

More generally, it's probably good to have an optional limit for OriginalDIMode, but I think it would be better if the limit could be passed as an integer instead of an enum. While 10000 may be a good default value for the builds you mentioned in the description, this is only with respect to your hardware setup and time constraints, and even then may be different for other builds (or even different optimization pipelines). Other users may have a higher or lower limit on the number of instructions they can afford to have processed by debugify, and should be able to specify this exactly via command line.

Also this is just my opinion and so I'd like to see what other reviewers think, but personally I think it would be best if the default setting was unlimited (so the limit is purely optional). When we're talking about metric-gathering, I think it's best to have the program do exactly what it says on the tin/what the user would expect it to do, and silently skipping instructions unless you pass an additional flag seems like potentially harmful unexpected behaviour. The documentation should be updated to include this option so that if a user does encounter performance issues then they will know that the option is there, but they will never unknowingly get incomplete results because they didn't know about this flag.

[ntesic]

Hi @StephenTozer, sorry for the delay, I was AFK.
Thanks for your comments.

In D115714#3243396, @StephenTozer wrote:

Performance seems like a serious issue when working with large projects here, but I have some questions/thoughts about this approach:

Definitely, and large projects are precious to us, for catching Debug Info Losses in the LLVM pipeline.

Is there any good reason to have an option to set the limit to zero? Unless I'm missing something, that would be equivalent to just not running debugify at all, which seems like a redundant option to have when debugify is an optional flag to begin with.

The zero limit was useful for comparing build times during development of these patches, but I agree that it is not very useful for users.

More generally, it's probably good to have an optional limit for OriginalDIMode, but I think it would be better if the limit could be passed as an integer instead of an enum. While 10000 may be a good default value for the builds you mentioned in the description, this is only with respect to your hardware setup and time constraints, and even then may be different for other builds (or even different optimization pipelines). Other users may have a higher or lower limit on the number of instructions they can afford to have processed by debugify, and should be able to specify this exactly via command line.

Explicitly setting the instruction limit number instead of using preset number, means the user should know its system constraints. This probably makes sense.

Also this is just my opinion and so I'd like to see what other reviewers think, but personally I think it would be best if the default setting was unlimited (so the limit is purely optional). When we're talking about metric-gathering, I think it's best to have the program do exactly what it says on the tin/what the user would expect it to do, and silently skipping instructions unless you pass an additional flag seems like potentially harmful unexpected behaviour. The documentation should be updated to include this option so that if a user does encounter performance issues then they will know that the option is there, but they will never unknowingly get incomplete results because they didn't know about this flag.

This patch came from our downstream use case, and in the general use case, all your comments make more sense. Thanks again!

[ntesic]

Set limitation granularity to the function level instead of instruction level.

• After latest update of D115622, we decide whether to use already collected Debug Info at the Function level, instead of the (whole) Module level. This is important, since the set of observed Functions in the same Module is not the equivalent for each pass in the LLVM pipeline. This update of the patch introduces the limit number of the observed Functions in the -verify-each-debuginfo-preserve pipeline.
• By default, consider unlimited number of Functions
• Set any number as a limit using the -debugify-func-limit option
• Rebase

Sorry for the big delay, and thanks @StephenTozer for the comments!

[djtodoro]

Please update https://llvm.org/docs/HowToUpdateDebugInfo.html#test-original-debug-info-preservation-in-optimizations with this. Other than that, looks good to me.

[ntesic]

• Add new argument usage to HowToUpdateDebugInfo documentation

Thanks @djtodoro!

llvm/lib/Transforms/Utils/Debugify.cpp

cl::opt<Level> DebugifyLevel(
     "debugify-level", cl::desc("Kind of debug info to add"),
     cl::values(clEnumValN(Level::Locations, "locations", "Locations only"),
                clEnumValN(Level::LocationsAndVariables, "location+variables",
                           "Locations and Variables")),
     cl::init(Level::LocationsAndVariables));
 
 raw_ostream &dbg() { return Quiet ? nulls() : errs(); }
 
+enum class LimitNum : unsigned int {
+  Zero = 0,
+  Default = 10000,
+  Unlimited = UINT_MAX
+};
+
+// Limit number of processed instructions during collectDebugInfoMetadata
+// and checkDebugInfoMetadata.
+cl::opt<LimitNum> DebugifyInstrLimit(
+    "debugify-instr-limit",
+    cl::desc("Set limit for the number of observed instructions per pass."),
+    cl::values(clEnumValN(LimitNum::Zero, "zero",
+                          "Don't process any instructions"),
+               clEnumValN(LimitNum::Default, "default",
+                          "Set limit to 10000 instructions"),
+               clEnumValN(LimitNum::Unlimited, "unlimited", "No limit")),
+    cl::init(LimitNum::Default));
+
 uint64_t getAllocSizeInBits(Module &M, Type *Ty) {
   return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty) : 0;
 }
 
 bool isFunctionSkipped(Function &F) {
   return F.isDeclaration() || !F.hasExactDefinition();
 }
 
bool llvm::collectDebugInfoMetadata(Module &M,
 
   LLVM_DEBUG(dbgs() << Banner << ": (before) " << NameOfWrappedPass << '\n');
 
   if (!M.getNamedMetadata("llvm.dbg.cu")) {
     dbg() << Banner << ": Skipping module without debug info\n";
     return false;
   }
 
+  unsigned int InstrCounter;
+  switch (DebugifyInstrLimit) {
+  case LimitNum::Default:
+    InstrCounter = static_cast<unsigned int>(LimitNum::Default);
+    break;
+  case LimitNum::Unlimited:
+    InstrCounter = static_cast<unsigned int>(LimitNum::Unlimited);
+    break;
+  default:
+    InstrCounter = static_cast<unsigned int>(LimitNum::Zero);
+  }
+
   // Visit each instruction.
   for (Function &F : Functions) {
     if (isFunctionSkipped(F))
       continue;
 
     // Collect the DISubprogram.
     auto *SP = F.getSubprogram();
     DebugInfoBeforePass.DIFunctions.insert({F.getName(), SP});
     if (SP) {
       LLVM_DEBUG(dbgs() << "  Collecting subprogram: " << *SP << '\n');
       for (const DINode *DN : SP->getRetainedNodes()) {
         if (const auto *DV = dyn_cast<DILocalVariable>(DN)) {
           DebugInfoBeforePass.DIVariables[DV] = 0;
         }
       }
     }
 
     for (BasicBlock &BB : F) {
       // Collect debug locations (!dbg) and debug variable intrinsics.
       for (Instruction &I : BB) {
         // Skip PHIs.
         if (isa<PHINode>(I))
           continue;
+        if (InstrCounter-- == 0)
+          return true;
 
         // Cllect dbg.values and dbg.declare.
         if (DebugifyLevel > Level::Locations) {
           if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I)) {
             if (!SP)
               continue;
             // Skip inlined variables.
             if (I.getDebugLoc().getInlinedAt())
bool llvm::checkDebugInfoMetadata(Module &M,
   if (!M.getNamedMetadata("llvm.dbg.cu")) {
     dbg() << Banner << ": Skipping module without debug info\n";
     return false;
   }
 
   // Map the debug info holding DIs after a pass.
   DebugInfoPerPass DebugInfoAfterPass;
 
+  unsigned int InstrCounter;
+  switch (DebugifyInstrLimit) {
+  case LimitNum::Default:
+    InstrCounter = static_cast<unsigned int>(LimitNum::Default);
+    break;
+  case LimitNum::Unlimited:
+    InstrCounter = static_cast<unsigned int>(LimitNum::Unlimited);
+    break;
+  default:
+    InstrCounter = static_cast<unsigned int>(LimitNum::Zero);
+  }
+
   // Visit each instruction.
   for (Function &F : Functions) {
+    if (InstrCounter == 0)
+      break;
     if (isFunctionSkipped(F))
       continue;
 
     // TODO: Collect metadata other than DISubprograms.
     // Collect the DISubprogram.
     auto *SP = F.getSubprogram();
     DebugInfoAfterPass.DIFunctions.insert({F.getName(), SP});
 
     if (SP) {
       LLVM_DEBUG(dbgs() << "  Collecting subprogram: " << *SP << '\n');
       for (const DINode *DN : SP->getRetainedNodes()) {
         if (const auto *DV = dyn_cast<DILocalVariable>(DN)) {
           DebugInfoAfterPass.DIVariables[DV] = 0;
         }
       }
     }
 
     for (BasicBlock &BB : F) {
+      if (InstrCounter == 0)
+        break;
       // Collect debug locations (!dbg) and debug variable intrinsics.
       for (Instruction &I : BB) {
         // Skip PHIs.
         if (isa<PHINode>(I))
           continue;
+        if (InstrCounter == 0)
+          break;
+        else
+          --InstrCounter;
 
         // Collect dbg.values and dbg.declares.
         if (DebugifyLevel > Level::Locations) {
           if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I)) {
             if (!SP)
               continue;
             // Skip inlined variables.
             if (I.getDebugLoc().getInlinedAt())

llvm/test/Transforms/Util/Debugify/loc-only-original-mode.ll

 ; RUN: opt < %s -deadargelim -enable-new-pm=false \
 ; RUN:     -verify-each-debuginfo-preserve \
 ; RUN:     -debugify-level=locations -S 2>&1 | FileCheck %s
 
+; RUN: opt < %s -deadargelim -enable-new-pm=false \
+; RUN:     -verify-each-debuginfo-preserve \
+; RUN:     -debugify-instr-limit=zero -S 2>&1 | FileCheck %s
+
+; RUN: opt < %s -deadargelim -enable-new-pm=false \
+; RUN:     -verify-each-debuginfo-preserve -debugify-instr-limit=unlimited \
+; RUN:     -S 2>&1 | FileCheck %s --check-prefix=UNLIMITED
+
 ;; Ensure that we check for DILocation potential issues only.
 ; CHECK-NOT: drops dbg.value()/dbg.declare()
+; UNLIMITED: drops dbg.value()/dbg.declare()
+
 
 target triple = "x86_64-unknown-linux-gnu"
 
 define dso_local i32 @fn2(i32 %l, i32 %k) !dbg !7 {
 entry:
   call void @llvm.dbg.value(metadata i32 %l, metadata !12, metadata !DIExpression()), !dbg !15
   call void @llvm.dbg.value(metadata i32 %k, metadata !13, metadata !DIExpression()), !dbg !15
   %call = call i32 (...) @fn3(), !dbg !16