[PATCH] D82129: [DebugInfo] Drop location ranges for variables which exist entirely outside the variable's scope

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

I'm a bit surprised how much code is necessary for this, but if there are clear size benefits of doing this, we should probably do it. Thanks for the detailed writeup!



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:99
+// compare instruction positions between blocks.
+static void numberInstructions(const MachineFunction &MF, OrderMap &Ordering) {
+  // We give meta instructions the same number as the peceding instruction
----------------
Nit: It's nice to use Doxygen-style /// comments here, since IDEs can format it specially and integrate it in help browsers etc.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:129
+
+// Check if the instruction range [StartMI, EndMI] intersects any instruction
+// range in Ranges. EndMI can be nullptr to indicate that the range is
----------------
Is this an inclusive interval on both sides or should this be [StartMI, EndMI)?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:201
+    for (auto EI = HistoryMapEntries.begin(), EE = HistoryMapEntries.end();
+         EI != EE; ++EI, ++StartIndex) {
+      // Only DBG_VALUEs can open location ranges so skip anything else.
----------------
this looks like it could be a range-based for loop?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:269
+    // removed.
+    for (size_t i = 0; i < HistoryMapEntries.size(); ++i)
+      if (HistoryMapEntries[i].isClosed())
----------------
range-based for?


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

https://reviews.llvm.org/D82129