[PATCH] Rewrite calculateDbgValueHistory to make it (hopefully) more transparent.
dblaikie at gmail.com
Mon May 5 09:58:44 PDT 2014
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:39
@@ +38,3 @@
+ // for indirect uses.
+ if (!MI.getNumOperands() == 3)
+ return None;
Should this just be an assertion?
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:41
@@ +40,3 @@
+ return None;
+ if (!MI.getOperand(0).isReg() || MI.getOperand(0).getReg() == 0U)
+ return None;
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?
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))
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?
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:78
@@ +77,3 @@
+ // Iterate over all varibales described by this register and add this
+ // instruction to their history, clobbering it.
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:102
@@ +101,3 @@
+// clobbered by @MI.
+static void handleClobberedRegs(RegDescribedVarsMap &RegVars,
+ const MachineInstr &MI,
'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))
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:125
@@ +124,3 @@
+ if (auto PrevReg = isDescribedByReg(*VarHistory.back()))
+ dropRegDescribedVar(RegVars, PrevReg.getValue(), Var);
You can use *PrevReg to get the value out of the Optional, if you like.
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:128
@@ -28,1 +127,3 @@
+ if (auto MIReg = isDescribedByReg(MI))
+ addRegDescribedVar(RegVars, MIReg.getValue(), Var);
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:140
@@ +139,3 @@
+ const auto LastMBBInstr = MBB.getLastNonDebugInstr();
+ bool SeenLastMBBInstr = LastMBBInstr == MBB.end();
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?
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:174
@@ +173,3 @@
+ if (&MBB == &MF->back() || LastMBBInstr == MBB.end())
+ clobberAllRegistersUses(RegVars, Result, *LastMBBInstr);
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?)
More information about the llvm-commits