[PATCH] Move logic for calculating DBG_VALUE history map into separate file/class.

[Alexey Samsonov]

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1444
@@ -1459,9 +1443,3 @@
         const MDNode *Var = MI->getDebugVariable();
-
-        // Variable is in a register, we need to check for clobbers.
-        if (isDbgValueInDefinedReg(MI))
-          LiveUserVar[MI->getOperand(0).getReg()] = Var;
-
-        // Check the history of this variable.
-        SmallVectorImpl<const MachineInstr *> &History = DbgValues[Var];
-        if (History.empty()) {
+        if (DbgValues.count(Var) == 0) {
+          // Create empty history for Var.
----------------
David Blaikie wrote:
> This seems like a slightly strange block.
> 
> Could you explain what the distinction between a variable not being in the DbgValues map, and it being in the map, but empty?
> 
> Also - "DbgValues[Var].clear()" is a little unclear - since the 'clear()' is actually not used - the condition above shows that the map didn't have the key until this point, so when it's created by op[] it should be empty anyway.
> 
> If this is the right semantic (see first question in this comment) I'd probably write it as:
> 
> auto IterBool = DbgValues.insert(std::make_pair(Var, SmallVector<...>()));
> if (IterPair.second)
>   UserVariables.push_back(Var);
> 
> (I wouldn't even mind rolling it into the if:
> 
> if (DbgValues.insert(std::make_pair(Var, SmallVector<...>())).second)
>   UserVariables.push_back(Var);
> 
> )
> 
> Could add a comment // ensure the Var is in the DbgValues map (using some more domain-specific wording, I haven't stared at this code long enough to understand why it needs to be in the map or what that means, etc)
> 
> 
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1452
@@ -1514,3 +1451,3 @@
         // the beginning of the function body.
         if (!MI->getFlag(MachineInstr::FrameSetup) &&
             (PrologEndLoc.isUnknown() && !MI->getDebugLoc().isUnknown()))
----------------
David Blaikie wrote:
> This could just be "else if", but might be better as a separate commit to keep this blame simple. (and it is simple - thank you for that, it's very easy to follow that the code has just been moved)
Sure, will do in a follow-up commit.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1468
@@ -1560,16 +1467,3 @@
 
-    // Make sure the final register assignments are terminated.
-    const MachineInstr *Prev = History.back();
-    if (Prev->isDebugValue() && isDbgValueInDefinedReg(Prev)) {
-      const MachineBasicBlock *PrevMBB = Prev->getParent();
-      MachineBasicBlock::const_iterator LastMI =
-          PrevMBB->getLastNonDebugInstr();
-      if (LastMI == PrevMBB->end())
-        // Drop DBG_VALUE for empty range.
-        History.pop_back();
-      else if (PrevMBB != &PrevMBB->getParent()->back()) {
-        // Terminate after LastMI.
-        History.push_back(LastMI);
-      }
-    }
-    // Request labels for the full history.
+    const MDNode *Var = I.first;
+    // The first mention of a function argument gets the FunctionBeginSym
----------------
David Blaikie wrote:
> I'd probably remove this local and just write: "DIVariable DV(I.first)"
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1474
@@ +1473,3 @@
+        getDISubprogram(DV.getContext()).describes(MF->getFunction()))
+      LabelsBeforeInsn[History[0]] = FunctionBeginSym;
+
----------------
David Blaikie wrote:
> I'd probably write History.front() (rather than History[0]), but reasonable people may disagree - just a thought.
Done.

http://reviews.llvm.org/D3573