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

[Alexey Samsonov]

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:37
@@ +36,3 @@
+  // the location of variable is unknown.
+  if (!MI.getOperand(0).isReg() || MI.getOperand(0).getReg() == 0U)
+    return None;
----------------
David Blaikie wrote:
> Alexey Samsonov wrote:
> > David Blaikie wrote:
> > > Is the first operand ever not a register? I thought that was an invariant of these intrinsics.
> > > 
> > > So it looks like isDescribedByReg boils down to "return MI.getOperand(0).getReg()", and some assertions?
> > > 
> > > (assuming the return type changes to "unsigned" rather than "Optional<unsigned>" - correctly returning 0/false when there is no register use (ie: this is a range terminator))
> > > 
> > > 
> > No. See getDebugLocValue() in DwarfDebug.cpp which actually distinguishes between different kinds of MachineOperands used as a first argument of DBG_VALUE. I've updated the comment in this function.
> OK. I see, it's either a register or a constant.
> 
> I'd still probably switch to just using an unsigned return value, dropping the Optional. Since zero is "not in a register" anyway.
> 
>   if (!MI.getOperand(0).isReg())
>     return 0;
>   return MI.getOperand(0).getReg();
> 
> I know it's a bit more subtle, using 0 as the "not in a register" state, but having two null states (Optional-with-value-zero and Optional-without-value) isn't ideal either... 
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:164
@@ +163,3 @@
+
+    // Make sure locations for register-described variables are valid only
+    // until the end of the basic block.
----------------
David Blaikie wrote:
> This is the normal for-loop exit condition, right?
> 
> Rather than breaking early (I found this hard to parse because I wasn't sure if it was exiting the loop early under some interesting condition - having to think about the addresses of elements, etc, was a bit taxing) could we just roll the condition into the clobberAllRegistersUses instead?
> 
> I suppose the condition will still be hard to pass, but I won't be trying to think about non-linear control flow at the same time.
> 
> Maybe the comment could be clarified here. "<existing comment> unless it's the last basic block, in which case let their liveness run off to the end of the function" or something?
>   if (!MBB.empty() && &MBB != &MF->back())
Agree. Done.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1271
@@ -1270,3 +1270,3 @@
                      << "\t" << *Begin << "\t" << *End << "\n");
-        if (End->isDebugValue())
+        if (End->isDebugValue() && End->getDebugVariable() == DV)
           SLabel = getLabelBeforeInsn(End);
----------------
David Blaikie wrote:
> What's the reason for these extra conditions? (the getDebugVariable() == DV checks here and below)
After this change, the location range for the variable DV:
1) starts at a DBG_VALUE instruction for DV
2) ends at another instruction. In particular, it can be DBG_VALUE instruction for some other variable.

http://reviews.llvm.org/D3597