[PATCH] Change representation of instruction ranges where variable is accessible.

[Alexey Samsonov]

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:117
@@ -116,3 @@
-                     const MachineInstr &MI) {
-  if (!VarHistory.empty()) {
-     const MachineInstr &Prev = *VarHistory.back();
----------------
David Blaikie wrote:
> Sorry, but I forget/don't understand why/how this code was no longer necessary.
> 
> Could you explain why it was necessary before your patch and how your patch makes it unnecessary?
> 
> Hmm, I think I see - it moved into startInstrRange, did it?
Not sure what logic you refer to. dropRegDescribedVar/addRegDescribedVar pair moved to calculateDbgValueHistory, so that now we have:
  * mark that the variable is no longer described by old register (if it has been).
  * start the new location range for a variable
  * mark that the variable is described by a new register (if necessary).

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1190
@@ +1189,3 @@
+
+      const MCSymbol *EndLabel = nullptr;
+      if (End != nullptr) {
----------------
David Blaikie wrote:
> You can drop this initialization, since the 3 cases below ensure the variable is initialized (this way, msan can catch bugs if that is ever violated)
Done

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1191
@@ +1190,3 @@
+      const MCSymbol *EndLabel = nullptr;
+      if (End != nullptr) {
+        EndLabel = getLabelAfterInsn(End);
----------------
David Blaikie wrote:
> This if/else if chain could be done without braces, if you like (seems to be the way of things in the LLVM codebase).
Done

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1368
@@ -1381,2 +1367,3 @@
+  // Find the end of the prologue.
   for (const auto &MBB : *MF) {
     for (const auto &MI : MBB) {
----------------
David Blaikie wrote:
> I'd probably drop the braces from all these - might be worth refactoring into a simple utility function to make this larger function more readable/simpler.
Done (and moved this function call closer to the place where PrologEndLoc is used for the first time).

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1385
@@ -1384,3 @@
-        assert(MI.getNumOperands() > 1 && "Invalid machine instruction!");
-        // Keep track of user variables in order of appearance. Create the
-        // empty history for each variable so that the order of keys in
----------------
David Blaikie wrote:
> Did the need for this ahead-of-time initialization of the MapVector go away in your change? How?
Yes, it went away. Need for ahead-of-time initialization is somewhat ugly, this really should be responsibility of calculateDbgValueHistory().

calculateDbgValueHistory() traverses all machine instructions in function, and the first time we see a DBG_VALUE for a new variable, it will
be added to map by the call to "startInstrRange()".


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1396
@@ +1395,3 @@
+    for (const auto &Range : Ranges) {
+      requestLabelBeforeInsn(Range.first);
+      if (Range.second)
----------------
David Blaikie wrote:
> I assume the case where the start of a range is non-null and the end of the range is null occurs for whole-function ranges? (we don't record the termination)
> 
> In that case, would it make sense to just record the location as an unbounded range (null for both start/end? No range? (I guess that'd be ambiguous with "variable is totally optimized away", though))
Not necessarily, see description of what a range is in DbgValueHistoryCalculator.h: if we have
  DBG_VALUE %r1 %noreg !"v"
  ...
  DBG_VALUE %r2 %noreg !"v"
the range for this first DBG_VALUE will not be explicitly terminated - we assume it's valid until the next range begins.

http://reviews.llvm.org/D3861