[PATCH] Rewrite calculateDbgValueHistory to make it (hopefully) more transparent.

[Alexey Samsonov]

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:39
@@ +38,3 @@
+  // for indirect uses.
+  if (!MI.getNumOperands() == 3)
+    return None;
----------------
David Blaikie wrote:
> Should this just be an assertion?
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:41
@@ +40,3 @@
+    return None;
+  if (!MI.getOperand(0).isReg() || MI.getOperand(0).getReg() == 0U)
+    return None;
----------------
David Blaikie wrote:
> When does this happen? (when is the first parameter not a register? or zero? (and what does zero mean - your comment above doesn't seem to describe this case, does it?))
> 
> Another assertion instead?
Yes, there can be instructions like:
  DBG_VALUE %noreg, 0, !"var"
which mark that debug location of "var" is now unknown. I've added a comment.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:44
@@ +43,3 @@
+  unsigned RegNo = MI.getOperand(0).getReg();
+  if (!MI.getOperand(1).isImm() &&
+      !(MI.getOperand(1).isReg() && MI.getOperand(1).getReg() == 0U))
----------------
David Blaikie wrote:
> This condition's still a bit hard to read (not sure if there's a better way, though) - but I also don't understand it. In both plain register and indirect uses, we'd still want to clobber the variable if the register in operand 0 was used, right? So why do we need to check whether this is indirect or direct?
Yeah, let's just get rid of this block :)

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:78
@@ +77,3 @@
+    return;
+  // Iterate over all varibales described by this register and add this
+  // instruction to their history, clobbering it.
----------------
David Blaikie wrote:
> variables
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:102
@@ +101,3 @@
+// clobbered by @MI.
+static void handleClobberedRegs(RegDescribedVarsMap &RegVars,
+                                const MachineInstr &MI,
----------------
David Blaikie wrote:
> 'handle' is fairly vague - maybe "terminateLiveRanges"? (maybe it's best to avoid "live ranges" terminology so as not to confuse it with real register allocation live ranges? Is there an alternative/better way we could refer to the ranges of a debug location? DWARF just refers to them as "location list entries" but the things we're building here aren't quite a DWARF location list entry yet... though they will be)
> 
> (I'd ever consider naming clobberRegisterUses/handlerClobberedRegs the same, just overloaded - just that one takes a specific register and the other takes a whole instruction & extracts the registers (ie: the functionality the have now, but they're really doing the same thing, kind of))
Sounds good, renamed both to clobberRegisterUses.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:125
@@ +124,3 @@
+    if (auto PrevReg = isDescribedByReg(*VarHistory.back()))
+      dropRegDescribedVar(RegVars, PrevReg.getValue(), Var);
+
----------------
David Blaikie wrote:
> You can use *PrevReg to get the value out of the Optional, if you like.
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:128
@@ -28,1 +127,3 @@
+  if (auto MIReg = isDescribedByReg(MI))
+    addRegDescribedVar(RegVars, MIReg.getValue(), Var);
 }
----------------
David Blaikie wrote:
> and here
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:140
@@ +139,3 @@
+    const auto LastMBBInstr = MBB.getLastNonDebugInstr();
+    bool SeenLastMBBInstr = LastMBBInstr == MBB.end();
+
----------------
David Blaikie wrote:
> This variable and its use seem confusing to me.
> 
> So the following loop loops over all MI in the MBB, does work if it's a debug_value, unless it's a trailing debug_value (a debug_value after all non-debug_value instructions in the block).
> 
> Do we have a lot of those? Is that worth a special case? I mean we should certainly prune zero-length location ranges, but they could arise from non-trailing debug_value anyway, right?
> 
Yes, this happens. Suppose we have the following machine basic block:

  <begin MBB>
  %R15 = ...
  DBG_VALUE %R15, %noreg, !"var"
  <end MBB>

Here we don't need to add zero-length location range for "var", and it's easier to just drop the range at this point.

In the history produced by this algorithm, location range is started by DBG_VALUE instruction and is terminated by some following instruction. If we start the new range by adding 
  DBG_VALUE %R15, %noreg, !"var"
to the history, how do we terminate it? Nothing follows it in the same machine basic block.


================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:174
@@ +173,3 @@
+    if (&MBB == &MF->back() || LastMBBInstr == MBB.end())
+      break;
+    clobberAllRegistersUses(RegVars, Result, *LastMBBInstr);
----------------
David Blaikie wrote:
> leaving a loop way down here seems a bit subtle - the idea here is that if we're in the last block of the function and there are no trailing debug instructions in the MBB?
> 
> I don't understand that second part of the condition, I suppose. I think I get the first part - if we're at the last block, let's not terminate anything, it'll be terminated by the end of the function anyway. (& I guess that produces correct debug info later on, even though we didn't properly terminate the range?)
Yes. We're not required to terminate DBG_VALUE entries in the last MBB - we just assume the location is valid until the end of the function. There is a code which deals with this in DwarfDebug::collectVariableInfo().

The second condition in the if-statement just checks if there is an instruction we can use to terminate location ranges. If there is no such instruction - no problem we shouldn't have started any new location ranges anyway, see "if (SeenLastMBBInstr && isDescribedByReg(MI))" above.

http://reviews.llvm.org/D3597