[PATCH] Rewrite calculateDbgValueHistory to make it (hopefully) more transparent.
Alexey Samsonov
samsonov at google.com
Mon May 5 14:16:46 PDT 2014
================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:39
@@ +38,3 @@
+ // for indirect uses.
+ if (!MI.getNumOperands() == 3)
+ return None;
----------------
David Blaikie wrote:
> Should this just be an assertion?
Done.
================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:41
@@ +40,3 @@
+ return None;
+ if (!MI.getOperand(0).isReg() || MI.getOperand(0).getReg() == 0U)
+ return None;
----------------
David Blaikie wrote:
> 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?
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.
================
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))
----------------
David Blaikie wrote:
> 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?
Yeah, let's just get rid of this block :)
================
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.
----------------
David Blaikie wrote:
> variables
Done.
================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:102
@@ +101,3 @@
+// clobbered by @MI.
+static void handleClobberedRegs(RegDescribedVarsMap &RegVars,
+ const MachineInstr &MI,
----------------
David Blaikie wrote:
> '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))
Sounds good, renamed both to clobberRegisterUses.
================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:125
@@ +124,3 @@
+ if (auto PrevReg = isDescribedByReg(*VarHistory.back()))
+ dropRegDescribedVar(RegVars, PrevReg.getValue(), Var);
+
----------------
David Blaikie wrote:
> You can use *PrevReg to get the value out of the Optional, if you like.
Done.
================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:128
@@ -28,1 +127,3 @@
+ if (auto MIReg = isDescribedByReg(MI))
+ addRegDescribedVar(RegVars, MIReg.getValue(), Var);
}
----------------
David Blaikie wrote:
> and here
Done.
================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:140
@@ +139,3 @@
+ const auto LastMBBInstr = MBB.getLastNonDebugInstr();
+ bool SeenLastMBBInstr = LastMBBInstr == MBB.end();
+
----------------
David Blaikie wrote:
> 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?
>
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.
================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:174
@@ +173,3 @@
+ if (&MBB == &MF->back() || LastMBBInstr == MBB.end())
+ break;
+ clobberAllRegistersUses(RegVars, Result, *LastMBBInstr);
----------------
David Blaikie wrote:
> 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?)
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.
http://reviews.llvm.org/D3597
More information about the llvm-commits
mailing list