Rewrite calculateDbgValueHistory to make it (hopefully) more transparent.

[samsonov]

Summary

This change preserves the original algorithm of generating history
for user variables, but makes it more clear.

High-level description of algorithm:
Scan all the machine basic blocks and machine instructions in the order
they are emitted to the object file. Do the following:

1. If we see a DBG_VALUE instruction, add it to the history of the

corresponding user variable. Keep track of all user variables, whose
locations are described by a register.

2. If we see a regular instruction, look at all the registers it clobbers,

and terminate the location range for all variables described by these registers.

3. At the end of the basic block, terminate location ranges for all

user variables described by some register.

(3) is too restrictive, and may result in a poor debug info.
For example, the variable location can be described by %rsp, or any other
register which is never clobbered in a function. I think that handling
this special case may improve situation for many real-life programs and
plan to address this in future commits.

[dblaikie]

Given that this is a substantial rewrite/change to the algorithm, would it be worthwhile to make it even more readable to begin with. What I have in mind (but perhaps it doesn't make sense - this is just an off-the-cuff idea) is trying to break down the new algorithm into some smaller functions with meaningful names that would make it easier to read through the code.

(since I'm going to have to read through the new code and understand it from scratch, since it doesn't map simply to the old algorithm, even if they produce the same result)

[samsonov]

Split the algorithm into several functions to make it more readable.

[samsonov]

Uploaded a new version. Hope it would be more understandable.

[probinson]

-----Original Message-----
From: [e-mail] [mailto:llvm-commits-
[e-mail]] On Behalf Of Alexey Samsonov

Hi dblaikie,

This change preserves the original algorithm of generating history
for user variables, but makes it more clear.

High-level description of algorithm:
Scan all the machine basic blocks and machine instructions in the order
they are emitted to the object file. Do the following:

1. If we see a DBG_VALUE instruction, add it to the history of the

corresponding user variable. Keep track of all user variables, whose
locations are described by a register.

2. If we see a regular instruction, look at all the registers it

clobbers,
and terminate the location range for all variables described by these
registers.

3. At the end of the basic block, terminate location ranges for all

user variables described by some register.

(3) is too restrictive, and may result in a poor debug info.
For example, the variable location can be described by %rsp, or any
other
register which is never clobbered in a function. I think that handling
this special case may improve situation for many real-life programs and
plan to address this in future commits.

Taking no stand on the patch, just on that last statement...

(3) is correctly conservative, because variable-in-register liveness
depends on control flow but debug-value ranges are expressed in terms
of physical order. Getting this truly right requires a dataflow
analysis to track liveness across block boundaries.

There may be special cases of small multi-block functions where some
variable lives in a register that is never clobbered, but I question
how often that will come up in real programs.

If a variable has a "home" on the stack, that's a different story.
Naively you can fall back to the stack location after a register
copy is clobbered and probably 90%-95% of the time you're fine.
--paulr

http://reviews.llvm.org/D3597

Files:

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

[dblaikie]

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

140

Not sure about making a local class here - I'd probably just pass the map around to a few functions for now - but I haven't read this all yet, perhaps it makes more sense the way you've got it here.

142

unnecessary default ctor - you can just omit this

146

Yeah, sort of feel like the RegDescribedVars could be passed through and this function (calculate) could be inlined into calculateDbgValueHistory. Then it wouldn't be weird about whether this DbgValueHistoryCalculator can be reused or not (it's not reused in its one use case, but it clears RegDescribedVars so it could be reused..)

148

This long condition could probably be inverted to a series of early returns with comments describing what each case represents.

As it stands it's a pretty opaque query of various operands and their types.

151

A const reference to an iterator is a bit weird. I'd probably use it by value (or is an iterator also an instruction, in which case I'd probably write it as "auto *" or "Instructior *")

152

This function could return Optional<unsigned> rather than bool + unsigned out parameter.

157

This 'if' is pretty long & the various things it's doing aren't exactly obvious - any way to split them out into some more named functions that would make it easier to read the high level algorithm? Maybe it's just some higher level comments, I'm not sure.

I'm not sure it would be better, but this could be inverted for less nesting:

if (!MI.isDebugValue()) {
  handleClobberedRegs(MI, TRI, Result);
  continue;
}

...

166

Is this a new feature of your algorithm, or did the old algorithm do this too?

176

The current LLVM convention is to not repeat the function name in the documentation comment (also doxy comments should be / not ). either omit it entirely, or if use \brief if the comment is brief (I forget when/why \brief is used/not used, though)

179

clobberVarsUsingRegister? clobberRegisterUses?

"RegDescribed" is a bit sub-optimal in general, I think, perhaps.

Maybe "clobberHistoryForInRegVars"?

200

I take it "RegDescribed" means "variables in registers" or just "variables not on the stack"? (what about variables that are indirected through a register?)

214

Sidenote: I wonder if MCRegAliasIterator could be a real iterator one day & this could use range-based for.

[samsonov]

Address David's comments.

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

140

Got rid of the class as suggested.

142

Done.

146

Done.

148

Done.

151

Dropped the reference.

152

Done.

157

Used last suggestion to reduce the nesting. Moved recalculating contents of RegDescribedVars to a separate function.

166

This data structure is different in my implementation. Current code just uses std::vector of size TRI->getNumRegs() instead of map. Yes, it assumes that there's at most one variable for each register.

176

Done (added \brief to comments).

179

Renamed to clobberRegisterUses/clobberAllRegisterUses.

200

It means variables in a register (DBG_VALUE %reg %reg), or indirected through register (DBG_VALUE %reg %imm). I can't think of a better name for it.

214

Ack.

[dblaikie]

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

39

Should this just be an assertion?

41

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?

44

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?

78

variables

102

'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))

125

You can use *PrevReg to get the value out of the Optional, if you like.

128

and here

146

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?

165

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

[samsonov]

Address more comments.

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

39

Done.

41

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.

44

Yeah, let's just get rid of this block :)

78

Done.

102

Sounds good, renamed both to clobberRegisterUses.

125

Done.

128

Done.

146

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.

165

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.

[dblaikie]

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

32

Is this ever called with a non_DBG_VALUE? It doesn't look like it. Should this just be an assertion?

37

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

146

"easier" in terms of computational work, but not necessarily easier in terms of being able to understand this algorithm, right?

I'd like to make this as legible as possible, because it is rather confusing. The comment you opened this patch with described a fairly clear algorithm, but that's not visible to me in the code, which is really unfortunate.

That being said, I know this is just a precursor to other refactoring, so I shouldn't dwell on making this "perfect".

The issue you describe here where there's no way to terminate an instruction would come up with intermediate values too:

DBG_VALUE %R15, %noreg, !"var"
DBG_VALUE %noreg, 0, !"var"
...

While I realize that's probably not a thing we ever create, it seems like having the algorithm tolerate that general case would probably make it more legible. (I don't know for sure, I haven't written it out that way)

Plucking ideas out of the air, maybe having a "pending start of range" list that is built whenever a DBG_VALUE is seen, but isn't added to the real list until a non-DBG_VALUE instruction is seen (that instruction would then form the start of the range).

That way the above example falls out by replacing (Or removing, in the case of the end-of-range %noreg) elements in the pending list, then throwing out the whole pending list when we get to the end, since anything in there never saw an instruction to start the range at. (and if we do see an instruction we build the start-of-real-range for each element in the pending list and clear out the pending list)

Alternatively if ranges can cover DBG_VALUE intrinsicts (since they start at the intrinsic, rather than the next instruction) - what actually breaks if they /only/ cover DBG_VALUE intrinsics? (can we just build these ranges, even if they're silly - to simplify the algorithm - or is there something that breaks if we do?)

[samsonov]

Address more comments.

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

32

It's used once for non-DBG_VALUE instruction, but I agree that it's easier to make an assertion here.

37

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.

146

OK, I'm not opposed to generating lots of empty location ranges to simplify the algorithm. The example you mention:

DBG_VALUE %R15, %noreg, !"var"
DBG_VALUE %noreg, 0, !"var"

should already be handled fine - both of these instruction appear in History for "var", and we will construct the range which starts before first instruction, and ends before the second one (it will of course be empty, as instructions are transient).

That said, for simplicity we may simply *always* terminate location ranges starting with register-using DBG_VALUE instruction by appending the last instruction in the MBB. Why not? This would require a minor change in contract, though: now History vector for Variable may contain arbitrary instructions - if the instruction is DBG_VALUE *for the same variable*, we assume it's a start of a new location range. Otherwise it's a clobbering instruction.

See the updated code.

(side note: probably it makes sense to factor out the code which turns History vector into actual DebugLocEntry list, as it is somewhat connected to the way we construct/represent the History vector).

[samsonov]

(gentle ping)

[samsonov]

David, could you take a look in case you have more comments?

[dblaikie]

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

37

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

164

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())

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1271

What's the reason for these extra conditions? (the getDebugVariable() == DV checks here and below)

[samsonov]

Address more comments.

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

37

Done.

164

Agree. Done.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1271

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.

[dblaikie]

Looks good - thanks for your patience!

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1271

Not to hold up the submission of this patch, but I can't quite grasp your explanation here.

Are you suggesting that this change (this particular part of your patch, or the patch in general) is changing behavior? It'd be helpful to keep the behavior change separate, if possible.

[samsonov]

Thanks for the review! Submitted as r209225.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1271

This shouldn't be a behavior change, we're slightly changing the assumptions about which instructions can be used for tracking the location of a variable (but this would result in the same labels and .debug_loc contents). Anyway, watching the bots.

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.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. The variables are listed in order of appearance.
+// For each user variable, keep a list of DBG_VALUE instructions for it
+// in the order of appearance. The list can also contain another
+// instructions, which are assumed to clobber the previous DBG_VALUE.
+// The variables are listed in order of appearance.
 typedef MapVector<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/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Target/TargetRegisterInfo.h"
+#include <algorithm>
+#include <map>
 
 #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,
+namespace {
+// Maps physreg numbers to the variables they describe.
+typedef std::map<unsigned, SmallVector<const MDNode *, 1>> RegDescribedVarsMap;
+}
+
+// \brief If @MI is a DBG_VALUE with debug value described by a
+// defined register, returns the number of this register.
+static Optional<unsigned> isDescribedByReg(const MachineInstr &MI) {
+  assert(MI.isDebugValue());
+  assert(MI.getNumOperands() == 3);
+  // If location of variable is described using a register, this register
+  // is always a first operand. Register 0 means that the location is now
+  // unknown.
+  if (!MI.getOperand(0).isReg() || MI.getOperand(0).getReg() == 0U)
+    return None;
+  // Otherwise, the variable is addressed using a register, either directly or
+  // indirectly.
+  return MI.getOperand(0).getReg();
+}
+
+// \brief Claim that @Var is not described by @RegNo anymore.
+static void dropRegDescribedVar(RegDescribedVarsMap &RegVars,
+                                unsigned RegNo, const MDNode *Var) {
+  const auto &I = RegVars.find(RegNo);
+  assert(I != RegVars.end());
+  auto &VarSet = I->second;
+  const auto &VarPos = std::find(VarSet.begin(), VarSet.end(), Var);
+  assert(VarPos != VarSet.end());
+  VarSet.erase(VarPos);
+  // Don't keep empty sets in a map to keep it as small as possible.
+  if (VarSet.empty())
+    RegVars.erase(I);
+}
+
+// \brief Claim that @Var is now described by @RegNo.
+static void addRegDescribedVar(RegDescribedVarsMap &RegVars,
+                               unsigned RegNo, const MDNode *Var) {
+  RegVars[RegNo].push_back(Var);
+}
+
+static void clobberVariableLocation(SmallVectorImpl<const MachineInstr *> &VarHistory,
+                                    const MachineInstr &ClobberingInstr) {
+  assert(!VarHistory.empty());
+  // DBG_VALUE we're clobbering should belong to the same MBB.
+  assert(VarHistory.back()->isDebugValue());
+  assert(VarHistory.back()->getParent() == ClobberingInstr.getParent());
+  VarHistory.push_back(&ClobberingInstr);
+}
+
+// \brief Terminate the location range for variables described by register
+// @RegNo by inserting @ClobberingInstr to their history.
+static void clobberRegisterUses(RegDescribedVarsMap &RegVars, unsigned RegNo,
+                                DbgValueHistoryMap &HistMap,
+                                const MachineInstr &ClobberingInstr) {
+  const auto &I = RegVars.find(RegNo);
+  if (I == RegVars.end())
+    return;
+  // Iterate over all variables described by this register and add this
+  // instruction to their history, clobbering it.
+  for (const auto &Var : I->second)
+    clobberVariableLocation(HistMap[Var], ClobberingInstr);
+  RegVars.erase(I);
+}
+
+// \brief Terminate the location range for all variables, described by registers
+// clobbered by @MI.
+static void clobberRegisterUses(RegDescribedVarsMap &RegVars,
+                                const MachineInstr &MI,
                                 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 (const auto &MI : *I) {
-      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);
-          }
-        }
-      }
+                                DbgValueHistoryMap &HistMap) {
+      unsigned RegNo = *AI;
+      clobberRegisterUses(RegVars, RegNo, HistMap, MI);
+
+// \brief Terminate the location range for all register-described variables
+// by inserting @ClobberingInstr to their history.
+static void clobberAllRegistersUses(RegDescribedVarsMap &RegVars,
+                                    DbgValueHistoryMap &HistMap,
+                                    const MachineInstr &ClobberingInstr) {
+  for (const auto &I : RegVars)
+    for (const auto &Var : I.second)
+      clobberVariableLocation(HistMap[Var], ClobberingInstr);
+  RegVars.clear();
+}
+
+// \brief Update the register that describes location of @Var in @RegVars map.
+static void
+updateRegForVariable(RegDescribedVarsMap &RegVars, const MDNode *Var,
+                     const SmallVectorImpl<const MachineInstr *> &VarHistory,
+                     const MachineInstr &MI) {
+  if (!VarHistory.empty()) {
+     const MachineInstr &Prev = *VarHistory.back();
+     // Check if Var is currently described by a register by instruction in the
+     // same basic block.
+     if (Prev.isDebugValue() && Prev.getDebugVariable() == Var &&
+         Prev.getParent() == MI.getParent()) {
+       if (auto PrevReg = isDescribedByReg(Prev))
+         dropRegDescribedVar(RegVars, *PrevReg, Var);
      }
   }
 
-  // Make sure the final register assignments are terminated.
-  for (auto &I : Result) {
-    SmallVectorImpl<const MachineInstr *> &History = I.second;
-    if (History.empty())
+  assert(MI.getDebugVariable() == Var);
+  if (auto MIReg = isDescribedByReg(MI))
+    addRegDescribedVar(RegVars, *MIReg, Var);
+}
+
+void calculateDbgValueHistory(const MachineFunction *MF,
+                              const TargetRegisterInfo *TRI,
+                              DbgValueHistoryMap &Result) {
+  RegDescribedVarsMap RegVars;
+
+  for (const auto &MBB : *MF) {
+    for (const auto &MI : MBB) {
+      if (!MI.isDebugValue()) {
+        // Not a DBG_VALUE instruction. It may clobber registers which describe
+        // some variables.
+        clobberRegisterUses(RegVars, MI, TRI, Result);
         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);
+      const MDNode *Var = MI.getDebugVariable();
+      auto &History = Result[Var];
+
+      if (!History.empty() && History.back()->isIdenticalTo(&MI)) {
+        DEBUG(dbgs() << "Coalescing identical DBG_VALUE entries:\n"
+                     << "\t" << History.back() << "\t" << MI << "\n");
+        continue;
       }
+
+      updateRegForVariable(RegVars, Var, History, MI);
+      History.push_back(&MI);
     }
+
+    // Make sure locations for register-described variables are valid only
+    // until the end of the basic block.
+    if (&MBB == &MF->back())
+      break;
+    if (!MBB.empty())
+      clobberAllRegistersUses(RegVars, Result, MBB.back());
   }
 }
 
 }

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (SmallVectorImpl<const MachineInstr *>::const_iterator
       if (HI + 1 == HE)
         // If Begin is the last instruction in History then its value is valid
         // until the end of the function.
         SLabel = FunctionEndSym;
       else {
         const MachineInstr *End = HI[1];
         DEBUG(dbgs() << "DotDebugLoc Pair:\n"
                      << "\t" << *Begin << "\t" << *End << "\n");
-        if (End->isDebugValue())
+        if (End->isDebugValue() && End->getDebugVariable() == DV)
           SLabel = getLabelBeforeInsn(End);
         else {
-          // End is a normal instruction clobbering the range.
+          // End is clobbering the range.
           SLabel = getLabelAfterInsn(End);
           assert(SLabel && "Forgot label after clobber instruction");
           ++HI;
         }
       }
 
       // The value is valid until the next DBG_VALUE or clobber.
       DebugLocEntry Loc(FLabel, SLabel, getDebugLocValue(Begin), TheCU);
for (auto &I : DbgValues) {
     // 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())
+      if (MI->isDebugValue() && MI->getDebugVariable() == DV)
         requestLabelBeforeInsn(MI);
       else
         requestLabelAfterInsn(MI);
     }
   }
 
   PrevInstLoc = DebugLoc();
   PrevLabel = FunctionBeginSym;