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

[samsonov]

Summary

No functionality change.

[samsonov]

Remove accidentally commited temp files.

[dblaikie]

Any particular motivation for this change? Is it a precursor to other work?

But yeah, putting this rather monolithic complexity in its own space is probably not a bad idea, I'd just like to understand the motivation/design a bit better.

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h

26

Immediate gut reaction is that this object has no state (presumably it doesn't mutate the MachineFunction or the TargetRegisterInfo - it just uses those to perform some computation) so it should be a free function.

Arguably it could still be in its own source file - it's certainly a lot of code - but I have an aversion to classes that are just containers for functions (we have namespaces for that).

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1449–1450

Unneeded braces on single-statement block. But I know some people prefer an "all or nothing" brace approach (if the 'if' has braces, then put braces on the 'else' too) - so that's up to you.

[echristo]

I like that it makes quite a bit of the code in DwarfDebug.cpp more compact, but it mostly just moves the complexity to another file. Is there a reason for the move or is it just an attempt at some nice cleanup? i.e. motivation?

[samsonov]

Sorry for the lack of context.

I'm preparing the ground for reviving http://reviews.llvm.org/D2658, which attempts to heavily refactor history calculation. It's probably a good idea to factor this logic out of beginFunction() anyway, as it doesn't require any knowledge of DwarfDebug contents.

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h

26

For now, when the class doesn't yet have a state, I can turn certainly turn it into a function for simplicity.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1449–1450

I'd prefer to leave braces here - else-block occupies multiple lines, has a comment etc.

[dblaikie]

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h

26

Yep - if you could just make this a function for now, that'd be great.

It helps me to see the different design requirements motivating design changes rather than having a design that, itself, lacks the justification/requirements.

[samsonov]

Use plain function instead of a stateless class.

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h

26

Done.

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1445–1447

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)

1452

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)

1468–1474

I'd probably remove this local and just write: "DIVariable DV(I.first)"

1474

I'd probably write History.front() (rather than History[0]), but reasonable people may disagree - just a thought.

[echristo]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1474

No objections, not necessarily for it either, but I like to think I'm reasonable and I don't disagree :)

[samsonov]

Address comments by David.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1445–1447

Done.

1452

Sure, will do in a follow-up commit.

1468–1474

Done.

1474

Done.

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1445–1447

Sorry, perhaps it got lost in the length of my original comment, but I still don't fully understand the need for this code. Could you explain it to me?

To quote my original comment:

"Could you explain what the distinction between a variable not being in the DbgValues map, and it being in the map, but empty?"

[samsonov]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1443

See the updated // comment here. We need to store both DbgValues map (MDNode->History) and a set of MDNodes in UserVariables in the order of appearance. There's no difference in missing MDNode and MDNode with an empty history, but we use the latter for constructing UserVariables (it will be constructed later anyway).

We could either fix a DbgValues map to ensure the order on keys, or introduce a temporary map we would use when constructing UserVariables here, or leave the code as is - I have no strong opinion about it.

[dblaikie]

Thanks for your patience, Alexey.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1443

Oh, sorry I missed the comment. Thanks for the details.

OK, that's a pity, though not your fault/relevant to this patch. (I may've perpetrated this oddity to address weird parameter reordering shenanigans - I'm not sure)

As a follow-up patch it'd be good to change DbgValues to a MapVector. Then we wouldn't have to pay the cost of lookup in collectVariableInfo where we loop over the UserVariables then look them up in DbgValues - we could just iterate directly.

And this code would become a little less weird - though it'd still need the comment about insertion order being important.

[samsonov]

Thanks for the review!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1443

Yep, I like the idea of using MapVector here. Will do in a following changes.

lib/CodeGen/AsmPrinter/CMakeLists.txt

 add_llvm_library(LLVMAsmPrinter
   AddressPool.cpp
   ARMException.cpp
   AsmPrinter.cpp
   AsmPrinterDwarf.cpp
   AsmPrinterInlineAsm.cpp
+  DbgValueHistoryCalculator.cpp
   DIE.cpp
   DIEHash.cpp
   DwarfAccelTable.cpp
   DwarfCFIException.cpp
   DwarfDebug.cpp
   DwarfException.cpp
   DwarfFile.cpp
   DwarfStringPool.cpp
   DwarfUnit.cpp
   ErlangGCPrinter.cpp
   OcamlGCPrinter.cpp
   Win64Exception.cpp
   WinCodeViewLineTables.cpp
   )
 
 add_dependencies(LLVMAsmPrinter intrinsics_gen)

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h

+//===-- llvm/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h ----*- C++ -*--===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef CODEGEN_ASMPRINTER_DBGVALUEHISTORYCALCULATOR_H_
+#define CODEGEN_ASMPRINTER_DBGVALUEHISTORYCALCULATOR_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
+
+namespace llvm {
+
+class MachineFunction;
+class MachineInstr;
+class MDNode;
+class TargetRegisterInfo;
+
+// For each user variable, keep a list of DBG_VALUE instructions in order.
+// The list can also contain normal instructions that clobber the previous
+// DBG_VALUE.
+typedef DenseMap<const MDNode *, SmallVector<const MachineInstr *, 4>>
+DbgValueHistoryMap;
+
+void calculateDbgValueHistory(const MachineFunction *MF,
+                              const TargetRegisterInfo *TRI,
+                              DbgValueHistoryMap &Result);
+}
+
+#endif

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

+//===-- llvm/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp -------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "DbgValueHistoryCalculator.h"
+#include "llvm/CodeGen/MachineBasicBlock.h"
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Target/TargetRegisterInfo.h"
+
+#define DEBUG_TYPE "dwarfdebug"
+
+namespace llvm {
+
+// Return true if debug value, encoded by DBG_VALUE instruction, is in a
+// defined reg.
+static bool isDbgValueInDefinedReg(const MachineInstr *MI) {
+  assert(MI->isDebugValue() && "Invalid DBG_VALUE machine instruction!");
+  return MI->getNumOperands() == 3 && MI->getOperand(0).isReg() &&
+         MI->getOperand(0).getReg() &&
+         (MI->getOperand(1).isImm() ||
+          (MI->getOperand(1).isReg() && MI->getOperand(1).getReg() == 0U));
+}
+
+void calculateDbgValueHistory(const MachineFunction *MF,
+                              const TargetRegisterInfo *TRI,
+                              DbgValueHistoryMap &Result) {
+  // LiveUserVar - Map physreg numbers to the MDNode they contain.
+  std::vector<const MDNode *> LiveUserVar(TRI->getNumRegs());
+
+  for (MachineFunction::const_iterator I = MF->begin(), E = MF->end(); I != E;
+       ++I) {
+    bool AtBlockEntry = true;
+    for (MachineBasicBlock::const_iterator II = I->begin(), IE = I->end();
+         II != IE; ++II) {
+      const MachineInstr *MI = II;
+
+      if (MI->isDebugValue()) {
+        assert(MI->getNumOperands() > 1 && "Invalid machine instruction!");
+
+        // Keep track of user variables.
+        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 = Result[Var];
+        if (!History.empty()) {
+          // We have seen this variable before. Try to coalesce DBG_VALUEs.
+          const MachineInstr *Prev = History.back();
+          if (Prev->isDebugValue()) {
+            // Coalesce identical entries at the end of History.
+            if (History.size() >= 2 &&
+                Prev->isIdenticalTo(History[History.size() - 2])) {
+              DEBUG(dbgs() << "Coalescing identical DBG_VALUE entries:\n"
+                           << "\t" << *Prev << "\t"
+                           << *History[History.size() - 2] << "\n");
+              History.pop_back();
+            }
+
+            // Terminate old register assignments that don't reach MI;
+            MachineFunction::const_iterator PrevMBB = Prev->getParent();
+            if (PrevMBB != I && (!AtBlockEntry || std::next(PrevMBB) != I) &&
+                isDbgValueInDefinedReg(Prev)) {
+              // Previous register assignment needs to terminate at the end of
+              // its basic block.
+              MachineBasicBlock::const_iterator LastMI =
+                  PrevMBB->getLastNonDebugInstr();
+              if (LastMI == PrevMBB->end()) {
+                // Drop DBG_VALUE for empty range.
+                DEBUG(dbgs() << "Dropping DBG_VALUE for empty range:\n"
+                             << "\t" << *Prev << "\n");
+                History.pop_back();
+              } else if (std::next(PrevMBB) != PrevMBB->getParent()->end())
+                // Terminate after LastMI.
+                History.push_back(LastMI);
+            }
+          }
+        }
+        History.push_back(MI);
+      } else {
+        // Not a DBG_VALUE instruction.
+        if (!MI->isPosition())
+          AtBlockEntry = false;
+
+        // Check if the instruction clobbers any registers with debug vars.
+        for (const MachineOperand &MO : MI->operands()) {
+          if (!MO.isReg() || !MO.isDef() || !MO.getReg())
+            continue;
+          for (MCRegAliasIterator AI(MO.getReg(), TRI, true); AI.isValid();
+               ++AI) {
+            unsigned Reg = *AI;
+            const MDNode *Var = LiveUserVar[Reg];
+            if (!Var)
+              continue;
+            // Reg is now clobbered.
+            LiveUserVar[Reg] = nullptr;
+
+            // Was MD last defined by a DBG_VALUE referring to Reg?
+            auto HistI = Result.find(Var);
+            if (HistI == Result.end())
+              continue;
+            SmallVectorImpl<const MachineInstr *> &History = HistI->second;
+            if (History.empty())
+              continue;
+            const MachineInstr *Prev = History.back();
+            // Sanity-check: Register assignments are terminated at the end of
+            // their block.
+            if (!Prev->isDebugValue() || Prev->getParent() != MI->getParent())
+              continue;
+            // Is the variable still in Reg?
+            if (!isDbgValueInDefinedReg(Prev) ||
+                Prev->getOperand(0).getReg() != Reg)
+              continue;
+            // Var is clobbered. Make sure the next instruction gets a label.
+            History.push_back(MI);
+          }
+        }
+      }
+    }
+  }
+
+  // Make sure the final register assignments are terminated.
+  for (auto &I : Result) {
+    SmallVectorImpl<const MachineInstr *> &History = I.second;
+    if (History.empty())
+      continue;
+
+    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);
+      }
+    }
+  }
+}
+
+}

lib/CodeGen/AsmPrinter/DwarfDebug.h

 //===----------------------------------------------------------------------===//
 
 #ifndef CODEGEN_ASMPRINTER_DWARFDEBUG_H__
 #define CODEGEN_ASMPRINTER_DWARFDEBUG_H__
 
 #include "DwarfFile.h"
 #include "AsmPrinterHandler.h"
 #include "DIE.h"
+#include "DbgValueHistoryCalculator.h"
 #include "DebugLocEntry.h"
 #include "DebugLocList.h"
 #include "DwarfAccelTable.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/FoldingSet.h"
class DwarfDebug : public AsmPrinterHandler {
 
   // Maps instruction with label emitted after instruction.
   DenseMap<const MachineInstr *, MCSymbol *> LabelsAfterInsn;
 
   // Every user variable mentioned by a DBG_VALUE instruction in order of
   // appearance.
   SmallVector<const MDNode *, 8> UserVariables;
 
-  // For each user variable, keep a list of DBG_VALUE instructions in order.
-  // The list can also contain normal instructions that clobber the previous
-  // DBG_VALUE.
-  typedef DenseMap<const MDNode *, SmallVector<const MachineInstr *, 4> >
-  DbgValueHistoryMap;
+  // History of DBG_VALUE and clobber instructions for each user variable.
   DbgValueHistoryMap DbgValues;
 
   // Previous instruction's location information. This is used to determine
   // label location to indicate scope boundries in dwarf debug info.
   DebugLoc PrevInstLoc;
   MCSymbol *PrevLabel;
 
   // This location indicates end of function prologue and beginning of function

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (const auto &VI : MMI->getVariableDbgInfo()) {
     RegVar->setFrameIndex(VI.Slot);
     if (!addCurrentFnArgument(RegVar, Scope))
       addScopeVariable(Scope, RegVar);
     if (AbsDbgVariable)
       AbsDbgVariable->setFrameIndex(VI.Slot);
   }
 }
 
-// Return true if debug value, encoded by DBG_VALUE instruction, is in a
-// defined reg.
-static bool isDbgValueInDefinedReg(const MachineInstr *MI) {
-  assert(MI->isDebugValue() && "Invalid DBG_VALUE machine instruction!");
-  return MI->getNumOperands() == 3 && MI->getOperand(0).isReg() &&
-         MI->getOperand(0).getReg() &&
-         (MI->getOperand(1).isImm() ||
-          (MI->getOperand(1).isReg() && MI->getOperand(1).getReg() == 0U));
-}
-
 // Get .debug_loc entry for the instruction range starting at MI.
 static DebugLocEntry::Value getDebugLocValue(const MachineInstr *MI) {
   const MDNode *Var = MI->getDebugVariable();
 
   assert(MI->getNumOperands() == 3);
   if (MI->getOperand(0).isReg()) {
     MachineLocation MLoc;
     // If the second operand is an immediate, this is a
void DwarfDebug::beginFunction(const MachineFunction *MF) {
   else
     Asm->OutStreamer.getContext().setDwarfCompileUnitID(TheCU->getUniqueID());
 
   // Emit a label for the function so that we have a beginning address.
   FunctionBeginSym = Asm->GetTempSymbol("func_begin", Asm->getFunctionNumber());
   // Assumes in correct section after the entry point.
   Asm->OutStreamer.EmitLabel(FunctionBeginSym);
 
-  const TargetRegisterInfo *TRI = Asm->TM.getRegisterInfo();
-  // LiveUserVar - Map physreg numbers to the MDNode they contain.
-  std::vector<const MDNode *> LiveUserVar(TRI->getNumRegs());
-
+  // Collect user variables, find the end of the prologue.
   for (MachineFunction::const_iterator I = MF->begin(), E = MF->end(); I != E;
        ++I) {
-    bool AtBlockEntry = true;
     for (MachineBasicBlock::const_iterator II = I->begin(), IE = I->end();
          II != IE; ++II) {
       const MachineInstr *MI = II;
-
       if (MI->isDebugValue()) {
         assert(MI->getNumOperands() > 1 && "Invalid machine instruction!");
-
-        // Keep track of user variables.
+        // Keep track of user variables in order of appearance. Store the set
+        // of variables we've already seen as a set of keys in DbgValues.
         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()) {
+        auto IterPair = DbgValues.insert(
+            std::make_pair(Var, SmallVector<const MachineInstr *, 4>()));
+        if (IterPair.second)
           UserVariables.push_back(Var);
-          // The first mention of a function argument gets the FunctionBeginSym
-          // label, so arguments are visible when breaking at function entry.
-          DIVariable DV(Var);
-          if (DV.isVariable() && DV.getTag() == dwarf::DW_TAG_arg_variable &&
-              getDISubprogram(DV.getContext()).describes(MF->getFunction()))
-            LabelsBeforeInsn[MI] = FunctionBeginSym;
-        } else {
-          // We have seen this variable before. Try to coalesce DBG_VALUEs.
-          const MachineInstr *Prev = History.back();
-          if (Prev->isDebugValue()) {
-            // Coalesce identical entries at the end of History.
-            if (History.size() >= 2 &&
-                Prev->isIdenticalTo(History[History.size() - 2])) {
-              DEBUG(dbgs() << "Coalescing identical DBG_VALUE entries:\n"
-                           << "\t" << *Prev << "\t"
-                           << *History[History.size() - 2] << "\n");
-              History.pop_back();
-            }
-
-            // Terminate old register assignments that don't reach MI;
-            MachineFunction::const_iterator PrevMBB = Prev->getParent();
-            if (PrevMBB != I && (!AtBlockEntry || std::next(PrevMBB) != I) &&
-                isDbgValueInDefinedReg(Prev)) {
-              // Previous register assignment needs to terminate at the end of
-              // its basic block.
-              MachineBasicBlock::const_iterator LastMI =
-                  PrevMBB->getLastNonDebugInstr();
-              if (LastMI == PrevMBB->end()) {
-                // Drop DBG_VALUE for empty range.
-                DEBUG(dbgs() << "Dropping DBG_VALUE for empty range:\n"
-                             << "\t" << *Prev << "\n");
-                History.pop_back();
-              } else if (std::next(PrevMBB) != PrevMBB->getParent()->end())
-                // Terminate after LastMI.
-                History.push_back(LastMI);
-            }
-          }
-        }
-        History.push_back(MI);
       } else {
-        // Not a DBG_VALUE instruction.
-        if (!MI->isPosition())
-          AtBlockEntry = false;
-
         // First known non-DBG_VALUE and non-frame setup location marks
         // the beginning of the function body.
         if (!MI->getFlag(MachineInstr::FrameSetup) &&
             (PrologEndLoc.isUnknown() && !MI->getDebugLoc().isUnknown()))
           PrologEndLoc = MI->getDebugLoc();
-
-        // Check if the instruction clobbers any registers with debug vars.
-        for (const MachineOperand &MO : MI->operands()) {
-          if (!MO.isReg() || !MO.isDef() || !MO.getReg())
-            continue;
-          for (MCRegAliasIterator AI(MO.getReg(), TRI, true); AI.isValid();
-               ++AI) {
-            unsigned Reg = *AI;
-            const MDNode *Var = LiveUserVar[Reg];
-            if (!Var)
-              continue;
-            // Reg is now clobbered.
-            LiveUserVar[Reg] = nullptr;
-
-            // Was MD last defined by a DBG_VALUE referring to Reg?
-            DbgValueHistoryMap::iterator HistI = DbgValues.find(Var);
-            if (HistI == DbgValues.end())
-              continue;
-            SmallVectorImpl<const MachineInstr *> &History = HistI->second;
-            if (History.empty())
-              continue;
-            const MachineInstr *Prev = History.back();
-            // Sanity-check: Register assignments are terminated at the end of
-            // their block.
-            if (!Prev->isDebugValue() || Prev->getParent() != MI->getParent())
-              continue;
-            // Is the variable still in Reg?
-            if (!isDbgValueInDefinedReg(Prev) ||
-                Prev->getOperand(0).getReg() != Reg)
-              continue;
-            // Var is clobbered. Make sure the next instruction gets a label.
-            History.push_back(MI);
-          }
-        }
       }
     }
   }
 
+  // Calculate history for local variables.
+  calculateDbgValueHistory(MF, Asm->TM.getRegisterInfo(), DbgValues);
+
+  // Request labels for the full history.
   for (auto &I : DbgValues) {
-    SmallVectorImpl<const MachineInstr *> &History = I.second;
+    const SmallVectorImpl<const MachineInstr *> &History = I.second;
     if (History.empty())
       continue;
 
-    // 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.
+    // The first mention of a function argument gets the FunctionBeginSym
+    // label, so arguments are visible when breaking at function entry.
+    DIVariable DV(I.first);
+    if (DV.isVariable() && DV.getTag() == dwarf::DW_TAG_arg_variable &&
+        getDISubprogram(DV.getContext()).describes(MF->getFunction()))
+      LabelsBeforeInsn[History.front()] = FunctionBeginSym;
+
     for (const MachineInstr *MI : History) {
       if (MI->isDebugValue())
         requestLabelBeforeInsn(MI);
       else
         requestLabelAfterInsn(MI);
     }
   }