[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/docs/HowToUpdateDebugInfo.rst

 .. code-block:: bash
 
   # Run the pass by checking original Debug Info preservation.
   $ opt -verify-debuginfo-preserve -pass-to-test sample.ll
 
   # Check the preservation of original Debug Info after each pass.
   $ opt -verify-each-debuginfo-preserve -O2 sample.ll
 
+Limit number of observed functions to speed up the analysis:
+
+.. code-block:: bash
+
+  # Test up to 100 functions (per compile unit) per pass.
+  $ opt -verify-each-debuginfo-preserve -O2 -debugify-func-limit=100 sample.ll
+
+Please do note that running ``-verify-each-debuginfo-preserve`` on big projects
+could be heavily time consuming. Therefore, we suggest using
+``-debugify-func-limit`` with a suitable limit number to prevent extremely long
+builds.
+
 Furthermore, there is a way to export the issues that have been found into
 a JSON file as follows:
 
 .. code-block:: bash
 
   $ opt -verify-debuginfo-preserve -verify-di-preserve-export=sample.json -pass-to-test sample.ll
 
 and then use the ``llvm/utils/llvm-original-di-preservation.py`` script

llvm/lib/Transforms/Utils/Debugify.cpp

 
 using namespace llvm;
 
 namespace {
 
 cl::opt<bool> Quiet("debugify-quiet",
                     cl::desc("Suppress verbose debugify output"));
 
+cl::opt<uint64_t> DebugifyFunctionsLimit(
+    "debugify-func-limit",
+    cl::desc("Set max number of processed functions per pass."),
+    cl::init(UINT_MAX));
+
 enum class Level {
   Locations,
   LocationsAndVariables
 };
 
 cl::opt<Level> DebugifyLevel(
     "debugify-level", cl::desc("Kind of debug info to add"),
     cl::values(clEnumValN(Level::Locations, "locations", "Locations only"),
bool llvm::collectDebugInfoMetadata(Module &M,
                                     StringRef NameOfWrappedPass) {
   LLVM_DEBUG(dbgs() << Banner << ": (before) " << NameOfWrappedPass << '\n');
 
   if (!M.getNamedMetadata("llvm.dbg.cu")) {
     dbg() << Banner << ": Skipping module without debug info\n";
     return false;
   }
 
+  uint64_t FunctionsCnt = DebugInfoBeforePass.DIFunctions.size();
   // Visit each instruction.
   for (Function &F : Functions) {
     // Use DI collected after previous Pass (when -debugify-each is used).
     if (DebugInfoBeforePass.DIFunctions.count(&F))
       continue;
 
     if (isFunctionSkipped(F))
       continue;
 
+    // Stop collecting DI if the Functions number reached the limit.
+    if (++FunctionsCnt >= DebugifyFunctionsLimit)
+      break;
     // Collect the DISubprogram.
     auto *SP = F.getSubprogram();
     DebugInfoBeforePass.DIFunctions.insert({&F, 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;
bool llvm::checkDebugInfoMetadata(Module &M,
   // Map the debug info holding DIs after a pass.
   DebugInfoPerPass DebugInfoAfterPass;
 
   // Visit each instruction.
   for (Function &F : Functions) {
     if (isFunctionSkipped(F))
       continue;
 
+    // Don't process functions without DI collected before the Pass.
+    if (!DebugInfoBeforePass.DIFunctions.count(&F))
+      continue;
     // TODO: Collect metadata other than DISubprograms.
     // Collect the DISubprogram.
     auto *SP = F.getSubprogram();
     DebugInfoAfterPass.DIFunctions.insert({&F, SP});
 
     if (SP) {
       LLVM_DEBUG(dbgs() << "  Collecting subprogram: " << *SP << '\n');
       for (const DINode *DN : SP->getRetainedNodes()) {

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-level=location+variables -S 2>&1 | FileCheck %s --check-prefix=CHECK-DROP
 
+; RUN: opt < %s -deadargelim -enable-new-pm=false \
+; RUN:     -verify-each-debuginfo-preserve \
+; RUN:     -debugify-func-limit=0 -S 2>&1 | FileCheck %s
+
+; RUN: opt < %s -deadargelim -enable-new-pm=false \
+; RUN:     -verify-each-debuginfo-preserve \
+; RUN:     -debugify-func-limit=2 -S 2>&1 | FileCheck %s --check-prefix=CHECK-DROP
+
+
 ; CHECK-NOT: drops dbg.value()/dbg.declare()
 ; CHECK-DROP: 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