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

[David Blaikie]

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

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:41
@@ +40,3 @@
+    return None;
+  if (!MI.getOperand(0).isReg() || MI.getOperand(0).getReg() == 0U)
+    return None;
----------------
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?

================
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))
----------------
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?

================
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.
----------------
variables

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:102
@@ +101,3 @@
+// clobbered by @MI.
+static void handleClobberedRegs(RegDescribedVarsMap &RegVars,
+                                const MachineInstr &MI,
----------------
'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))

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

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

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:140
@@ +139,3 @@
+    const auto LastMBBInstr = MBB.getLastNonDebugInstr();
+    bool SeenLastMBBInstr = LastMBBInstr == MBB.end();
+
----------------
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?


================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:174
@@ +173,3 @@
+    if (&MBB == &MF->back() || LastMBBInstr == MBB.end())
+      break;
+    clobberAllRegistersUses(RegVars, Result, *LastMBBInstr);
----------------
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?)

http://reviews.llvm.org/D3597