[PATCH] D118460: [DebugInfo][InstrRef] Use a depth-first search to reduce the lifetime of tracking information

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

In D118460#3284107 <https://reviews.llvm.org/D118460#3284107>, @Orlando wrote:

> Would it be worth caching `getBlocksForScope` results? If I'm reading this right it looks like it's called a few times with what should be the same inputs and outputs in a few places in the pass?

wdyt about this? I'm happy for that to come as a separate patch if you agree, and it looks like @dblaikie's comments have been addressed, so LGTM if you make clang-format bot happy.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:2986
+    auto DILocIt = ScopeToDILocation.find(WS);
+    if (HighestDFSIn <= WS->getDFSIn() && DILocIt != ScopeToDILocation.end()) {
+      const DILocation *DILoc = DILocIt->second;
----------------
jmorse wrote:
> Orlando wrote:
> > Repeating offline discussion so it's noted on the review -- it looks as though it might be possible to simplify this by removing the `HighestDFSIn` check and moving the body of this block into the `if (ChildNum < (ssize_t)Children.size()) { ... }` block below.
> I went to implement this and then realised: the block you mention only runs for scopes where there are child scopes, and the "else" block only runs after child scopes are visited. I think this counter/ratchet needs to continue to exist so that scopes are always visited, and in the correct order.
Aha, SGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118460/new/

https://reviews.llvm.org/D118460