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

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

Orlando marked 2 inline comments as done.
Orlando added a comment.

Hi @aprantl, thanks for taking a look.



================
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
----------------
aprantl wrote:
> Is this an inclusive interval on both sides or should this be [StartMI, EndMI)?
This is meant to be inclusive on both sides, yeah. Looks like the second `if` inside the function body should actually be:
```
if (EndMI && !isBefore(RangesI->second, EndMI, Ordering))
   return RangesI;
```
I'll fix this mistake. Though luckily this won't have made a noticeable difference since AFAICT we're only incorrectly dropping 0 length location ranges because of it, and these wouldn't be emitted anyway.


================
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.
----------------
aprantl wrote:
> this looks like it could be a range-based for loop?
I agree that this loop is a little bit ugly; I'm doing it this way to update `StartIndex`.


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

https://reviews.llvm.org/D82129