[DebugInfo][RemoveDIs] Don't pointlessly scan funcs for debug-info

[jmorse]

The utility functions this patch modifies are part of cleanly transitioning from a context where we use dbg.value intrinsics to one where we use DPValue objects to record debug-info, and back again. However, this is a waste of time in non-debug builds (i.e. no -g on the command line). We still have to call the function on all blocks though to set the IsNewDbgInfoFormat flag.

To reduce the overhead of this, test whether there's any debug-info in the function by checking whether the function has a DISubprogram, and pass a flag down to the utility functions indicating whether they can skip the scan.

It feels a bit dumb to me now that we're scanning and setting a flag in a load of blocks when we don't have to -- however it's been really valuable during development for working out where spurious dbg.value intrinsics leak into a RemoveDIs context. Happily we'll be able to just delete this flag entirely when RemoveDIs lands and sticks, and the conversion routines will eventually be pushed down into the debug-info autoupgrade path.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Jeremy Morse (jmorse)

Changes

The utility functions this patch modifies are part of cleanly transitioning from a context where we use dbg.value intrinsics to one where we use DPValue objects to record debug-info, and back again. However, this is a waste of time in non-debug builds (i.e. no -g on the command line). We still have to call the function on all blocks though to set the IsNewDbgInfoFormat flag.

To reduce the overhead of this, test whether there's any debug-info in the function by checking whether the function has a DISubprogram, and pass a flag down to the utility functions indicating whether they can skip the scan.

It feels a bit dumb to me now that we're scanning and setting a flag in a load of blocks when we don't have to -- however it's been really valuable during development for working out where spurious dbg.value intrinsics leak into a RemoveDIs context. Happily we'll be able to just delete this flag entirely when RemoveDIs lands and sticks, and the conversion routines will eventually be pushed down into the debug-info autoupgrade path.

---

Full diff: https://github.com/llvm/llvm-project/pull/79327.diff

3 Files Affected:

• (modified) llvm/include/llvm/IR/BasicBlock.h (+6-2)
• (modified) llvm/lib/IR/BasicBlock.cpp (+10-2)
• (modified) llvm/lib/IR/Function.cpp (+4-2)

diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index a72b68d867f36e7..39ac9a5cf4a5e40 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -81,12 +81,16 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
   /// intrinsics into DPMarker / DPValue records. Deletes all dbg.values in
   /// the process and sets IsNewDbgInfoFormat = true. Only takes effect if
   /// the UseNewDbgInfoFormat LLVM command line option is given.
-  void convertToNewDbgValues();
+  /// \param HasNoDebugInfo Flag indicating the scan of instructions should be
+  /// skipped as the function is known to have no debug-info.
+  void convertToNewDbgValues(bool HasNoDebugInfo = false);
 
   /// Convert variable location debugging information stored in DPMarkers and
   /// DPValues into the dbg.value intrinsic representation. Sets
   /// IsNewDbgInfoFormat = false.
-  void convertFromNewDbgValues();
+  /// \param HasNoDebugInfo Flag indicating the scan of instructions should be
+  /// skipped as the function is known to have no debug-info.
+  void convertFromNewDbgValues(bool HasNoDebugInfo = false);
 
   /// Ensure the block is in "old" dbg.value format (\p NewFlag == false) or
   /// in the new format (\p NewFlag == true), converting to the desired format
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index dca528328384701..e9d15989b6e5e8e 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -60,13 +60,17 @@ DPMarker *BasicBlock::createMarker(InstListType::iterator It) {
   return DPM;
 }
 
-void BasicBlock::convertToNewDbgValues() {
+void BasicBlock::convertToNewDbgValues(bool HasNoDebugInfo) {
   // Is the command line option set?
   if (!UseNewDbgInfoFormat)
     return;
 
   IsNewDbgInfoFormat = true;
 
+  // If the caller knows there's no debug-info in this function, do nothing.
+  if (HasNoDebugInfo)
+    return;
+
   // Iterate over all instructions in the instruction list, collecting dbg.value
   // instructions and converting them to DPValues. Once we find a "real"
   // instruction, attach all those DPValues to a DPMarker in that instruction.
@@ -93,10 +97,14 @@ void BasicBlock::convertToNewDbgValues() {
   }
 }
 
-void BasicBlock::convertFromNewDbgValues() {
+void BasicBlock::convertFromNewDbgValues(bool HasNoDebugInfo) {
   invalidateOrders();
   IsNewDbgInfoFormat = false;
 
+  // If the caller knows there's no debug-info in this function, do nothing.
+  if (HasNoDebugInfo)
+    return;
+
   // Iterate over the block, finding instructions annotated with DPMarkers.
   // Convert any attached DPValues to dbg.values and insert ahead of the
   // instruction.
diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
index 22e2455462bf445..58e180c2fd6a519 100644
--- a/llvm/lib/IR/Function.cpp
+++ b/llvm/lib/IR/Function.cpp
@@ -83,15 +83,17 @@ static cl::opt<unsigned> NonGlobalValueMaxNameSize(
 
 void Function::convertToNewDbgValues() {
   IsNewDbgInfoFormat = true;
+  bool HasNoDebugInfo = getSubprogram() == nullptr;
   for (auto &BB : *this) {
-    BB.convertToNewDbgValues();
+    BB.convertToNewDbgValues(HasNoDebugInfo);
   }
 }
 
 void Function::convertFromNewDbgValues() {
   IsNewDbgInfoFormat = false;
+  bool HasNoDebugInfo = getSubprogram() == nullptr;
   for (auto &BB : *this) {
-    BB.convertFromNewDbgValues();
+    BB.convertFromNewDbgValues(HasNoDebugInfo);
   }
 }
 

[OCHyams]

Seems sensible, LGTM