[PATCH] D11933: Extending debug ranges

[Vikram TV via llvm-commits]

tvvikram added a comment.

In http://reviews.llvm.org/D11933#234313, @aprantl wrote:

> In http://reviews.llvm.org/D11933#234164, @tvvikram wrote:
>
> > In http://reviews.llvm.org/D11933#233887, @aprantl wrote:
> >
> > > Thanks so far. I added a bunch of more detailed comments inline.
> >
> >
> > I will make the suggested changes and submit a new patch.
> >
> > I am not sure about the name for the new pass. Please suggest.
> >  I have a couple of names though - **ImproveDebugValues.cpp**, **DebugValueDFA.cpp** (DFA: DataFlow Analysis). Please confirm.
>
>
> To prevent you from having to rename the file and pass more than once, let's defer this decision until the very end.


Thanks.

> Another possible name is "DebugValuePropagation", although "LiveDebugValues", or "DebugValueLiveness" would actually be the most fitting, because we are doing a Liveness analysis. This conflicts with the existing LiveDebugVariables pass that consumes all DBG_VALUEs that describe vregs, builds a side-table that is used during register allocation and writes out new DBG_VALUEs with real registers after regalloc is done. Perhaps we could rename the old LiveDebugVariables to "MaterializeDebugValues", or "RegallocDebugValues?


The new pass could be a place to do generic code changes related to DBG_VALUE instructions like inferring multiple locations, inserting missing DBG_VALUE instructions if possible, etc. If that is the case, I think we should name the pass with a generic name. Otherwise, I am okay with the name changes you suggested.

> To recap, my vision is that:

> 

> - the old LiveDebugVariables becomes a simple and fast BB-local pass that emits the bare minimum of DBG_VALUEs and

> - this new pass then performs the control-flow-aware liveness DFA that extends their range beyond BB boundaries by inserting enough DBG_VALUEs that

> - the DWARF backend may again assume that DBG_VALUEs are BB-local (and only performs a very simple merging of adjacent ranges).

> 

>   This way we will end up with small, understandable, and (via MIR) testable passes with a well-defined scope that do one task well.


Thanks for the high level view.


================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:16
@@ +15,3 @@
+// b. Try to infer the presence of a debug variable at multiple locations from
+//    move instructions (TODO).
+// c. Add missing DBG_VALUE instructions, if possible (TODO).
----------------
aprantl wrote:
> Please only document what the code does and don't talk about future extensions.
Removed.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:18
@@ +17,3 @@
+// c. Add missing DBG_VALUE instructions, if possible (TODO).
+// d. Any DBG_VALUE instruction related code.
+//
----------------
aprantl wrote:
> ???
Removed. Initially, this file was meant to handle any code changes related to DBG_VALUE instructions.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:159
@@ +158,3 @@
+// redundant ones. The lookup is only till the first non-DBG_VALUE instr.
+// Notify caller by returning the next instruction to start with.
+MachineBasicBlock::iterator
----------------
aprantl wrote:
> Better: \return the instruction following \c MI.
I am not sure about it. MI could already be the last instr in MBB. Anyway, I have removed this routine for now.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:161
@@ +160,3 @@
+MachineBasicBlock::iterator
+ExtendDebugRangeLocation::RemoveRedundantDbgVals(MachineInstr &MI) {
+
----------------
aprantl wrote:
> This could be split out into a separate patch and/or tested separately.
Removed. I will submit a separate patch for this.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:179
@@ +178,3 @@
+    bool Unique = true; // Is the instruction unique? Currently yes.
+    for (auto u = UniqueInstrs.begin(), ue = UniqueInstrs.end(); u != ue; ++u) {
+      MachineInstr *U = *u;
----------------
aprantl wrote:
> Could this be written as a ranged-based for?
I was not updating the iterators after erasing 'u' at line 221. I have rewritten the iterators but could not convert it to 'range-based for'.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:289
@@ +288,3 @@
+    // as index into History. The full variables including the
+    // piece expressions are attached to the MI.
+    const DILocalVariable *RawVar = MI.getDebugVariable();
----------------
aprantl wrote:
> Why is this correct?
Unintended comment. Removed.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:306
@@ +305,3 @@
+    // Add Var to OpenRanges from this DBG_VALUE
+    // TODO: Currently handles DBG_VALUE which has only reg as location
+    if (isDescribedByReg(MI))
----------------
aprantl wrote:
> Are constants and memory locations handled in a safe/correct way?
> 
AFAIK, yes as only register locations are handled now.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:333
@@ +332,3 @@
+  // Early return. Multiple locations need not track OutgoingLocs (code below).
+  if (HandleMultipleLocations)
+    return;
----------------
aprantl wrote:
> Please remove anything multiple locations from this patch it makes reviewing more complicated than necessary.
Sorry. I had missed out removing this when I separated out the multiple locations patch. Removed.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:359
@@ +358,3 @@
+// same location.
+// TODO: Currently handling when n = {1, 2}. Can mostly do better by having
+// OutgoingLocs in bitvector
----------------
aprantl wrote:
> I don't understand the first sentence.
Removed. I meant that the current implementation support bbs with at most 2 predecessors.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:393
@@ +392,3 @@
+  for (VarLocList::iterator OLi = OutgoingLocs.begin(),
+        OLe = OutgoingLocs.end(); OLi != OLe; OLi++) {
+    for (VarLocList::iterator OLii = OutgoingLocs.begin(),
----------------
aprantl wrote:
> range-based for?
The comparison at line 397 prevented me in converting to 'range based for'.


http://reviews.llvm.org/D11933