[PATCH] D73725: [DebugInfo] Avoid a quadratic-complexity corner case in LiveDebugValues

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
jmorse added reviewers: aprantl, vsk, bjope, probinson.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

Long ago [0], it was identified that part of LiveDebugValues had worst-case quadratic complexity. We test [1] whether a variable location would still be in scope when propagated into a block, to avoid needlessly propagating locations. If you have a block of DBG_VALUEs, all for different variables, then the LexicalScopes::dominates method will scan each instruction for each variable location, which is O(n^2). It doesn't appear to have been judged a high risk at the time.

We've run into a circumstance where this happens, once again with large amounts of inlining into a large basic block. To avoid the quadratic complexity, I think we can rely on the scope ranges that are computed in each LexicalScope object [2]. These identify spans of the function that the lexical scope contains. Crucially, these spans cover all subscopes [3] as well. This patch changes the LexicalScopes::dominates method to, rather than testing each instruction in a block to see if it's dominated, instead test whether this block is in the set of dominated instructions blocks. (Which is a mouthful). This also involves fixing the getMachineBasicBlocks method, which didn't seem to be aware that each span could cover multiple blocks.

This patch eliminates the performance problem we encountered, and the complexity of LexicalScopes::dominates becomes linear with the size of the scope (measured in blocks), rather than the number of instructions in this block. The worst case would now be a circumstance where there are a large number of variable locations in a lexical scope that covers a large number of blocks (meaning we had to iterate over many blocks on each call to dominates). This, I believe, is not a circumstance that the inliner tries to create, as opposed to "many inlined scopes in one large block", which it does.

I tried building clang-3.4 with this patch applied, and there was no observable change in compile time (I guess it doesn't hit the worst case). I also tried building a stage2 build of clang with itself, with both implementations of LexicalScopes::dominates, comparing their answers. This showed up that the (current) implementation of dominates will interpret a metainstruction as being dominated , instead of ignoring it like the rest of LexicalScopes [4]. Aside from that, both implementations of dominates agreed in all their results.

No test because this is a performance patch, although I can add one for the "don't consider metainstructions in scope dominance queries" matter if that's important.

[0] https://reviews.llvm.org/D24994#inline-215341
[1] https://github.com/llvm/llvm-project/blob/3ae11b42818363f70b3c6b0246bb617e35709c58/llvm/lib/CodeGen/LiveDebugValues.cpp#L1328
[2] https://github.com/llvm/llvm-project/blob/3ae11b42818363f70b3c6b0246bb617e35709c58/llvm/include/llvm/CodeGen/LexicalScopes.h#L129
[3] https://github.com/llvm/llvm-project/blob/3ae11b42818363f70b3c6b0246bb617e35709c58/llvm/include/llvm/CodeGen/LexicalScopes.h#L76
[4] https://github.com/llvm/llvm-project/blob/3ae11b42818363f70b3c6b0246bb617e35709c58/llvm/lib/CodeGen/LexicalScopes.cpp#L92


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73725

Files:
  llvm/lib/CodeGen/LexicalScopes.cpp


Index: llvm/lib/CodeGen/LexicalScopes.cpp
===================================================================
--- llvm/lib/CodeGen/LexicalScopes.cpp
+++ llvm/lib/CodeGen/LexicalScopes.cpp
@@ -291,9 +291,20 @@
     return;
   }
 
+  // The scope ranges can cover multiple basic blocks in each span. Iterate over
+  // all blocks (in the order they are in the function) until we reach the one
+  // containing the end of the span.
   SmallVectorImpl<InsnRange> &InsnRanges = Scope->getRanges();
-  for (auto &R : InsnRanges)
-    MBBs.insert(R.first->getParent());
+  for (auto &R : InsnRanges) {
+    auto CurMBBIt = R.first->getParent()->getIterator();
+    auto EndBBIt = R.second->getParent()->getIterator();
+    do {
+      MBBs.insert(&*CurMBBIt);
+      if (CurMBBIt == EndBBIt)
+        break;
+      CurMBBIt++;
+    } while (true);
+  }
 }
 
 /// dominates - Return true if DebugLoc's lexical scope dominates at least one
@@ -308,14 +319,12 @@
   if (Scope == CurrentFnLexicalScope && MBB->getParent() == MF)
     return true;
 
-  bool Result = false;
-  for (auto &I : *MBB) {
-    if (const DILocation *IDL = I.getDebugLoc())
-      if (LexicalScope *IScope = getOrCreateLexicalScope(IDL))
-        if (Scope->dominates(IScope))
-          return true;
-  }
-  return Result;
+  // Fetch all the blocks in DLs scope. Because the range / block list also
+  // contain any subscopes, any instruction that DL dominates can be found
+  // in the block set.
+  SmallPtrSet<const MachineBasicBlock *, 32> Set;
+  getMachineBasicBlocks(DL, Set);
+  return Set.count(MBB) != 0;
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73725.241491.patch
Type: text/x-patch
Size: 1638 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200130/e59202da/attachment.bin>