[PATCH] Prevent the creation of empty location list ranges.

[Frederic Riss]

In http://reviews.llvm.org/D6497#12, @samsonov wrote:

> Finally looked at this patch, sorry for the delay.
>
> First of all, I dislike the extension of `InstrRange`  structure, and adding the extra list to DbgValueHistoryMap. After we returned from `calculateDbgValueHistory()`, we don't need and never use InstrRange::NonEmpty pointers, and the list, so it's not nice we leave this extra data lying around.


I can understand that and especially the liveness of the NonEmpty field bothers me. However, let me restate my goal: I wanted to find a way to keep the pass at the same algorithmic complexity. DbgValueHistoryCalculator has already shown up on profiles and I wanted to avoid introducing new potentially costly bookkeeping logic. That being said, I might be too careful. (And I even failed at that, because the destructor of the list<> will do a O(<number of Dbg_VALUE>) walk anyway).

> The comment about InstrRange explicitly tells that instruction range *may not* be terminated - it means the range is assumed to be valid either until the start of the next range,


(not really relating to that actual patch: I do not see why the beginning of a variable range should close the previous one for that variable. A variable might be present in 2 registers at some point it is even explicitly stated in the Dwarf standard. I even think you can't get the end result right if you try to prevent this from happening.)

> or until the end of function. However, you pop_back() "empty" instruction ranges only in endInstrRange() function. It means you may still end up with empty ranges if they were never closed.


This is true, however the empty ranges could only appear at the very end of the function which is less of an issue. For an empty range to happen there, something would need to have generated a DBG_VALUE after the last instruction of the function (Is that even possible? isn't there a terminator requirement like at the higher level?). But yes, this pass runs so late (meaning the DBG_VALUEs could have been mishandled by so many things) that we need to take care of that case if we want to be exhaustive.

> Can we do the following instead: have another member:

> 

>   std::map<const MDNode *, InstrRange> emptyRanges;

> 

> which would describe the current, opened and empty, range for a given variable.


You can have multiple open ranges for a variable (range entries describing constants will never be closed for example). I do not like the limitation to only one pending range. Thus we'd need to make the map value type a list.

>   It means that smth. like

>    const InstrRange& getCurrentRangeForVar(const MDNode *Var);

> 

> would return either the entry from `emptyRanges`, or the last value of `VarInstrRanges` vector.

>  Then `startInstrRange` would simply add new range, and `markOpenRangesNonEmpty()` would move the contents of `emptyRanges` to `VarInstrRanges`. At the end of calculateDbgValueHistory, we can just discard the contents of `emptyRanges`.


I'll try to use a simpler approach with a map<MDnode *, list<InstrRange>> if everybody agrees that the additional bookkeeping shouldn't be an issue.


================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:206
@@ +205,3 @@
+
+        if (!MI.isPosition() && !MI.isImplicitDef() && !MI.isKill())
+          Result.markOpenRangesNonEmpty();
----------------
samsonov wrote:
> Hm, this code is run after the register allocator, can you use isTransient() here? If no, maybe we can introduce `MachineInstr::isPseudoInstruction()` and use it in `MachineInstr::isTransient()` instead?
I reused the way AsmPrinter::EmitFunctionBody() counts the real emitted instructions. isTransient() seems to DTRT, I could use that.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h:75
@@ -45,3 +74,3 @@
   bool empty() const { return VarInstrRanges.empty(); }
-  void clear() { VarInstrRanges.clear(); }
+  void clear() { VarInstrRanges.clear(); SeenInstructionInRange.resize(1); }
   InstrRangesMap::const_iterator begin() const { return VarInstrRanges.begin(); }
----------------
samsonov wrote:
> Note that this can leave your `SeenInstructionInRange` list have a single "true" value. This would probably still work, though.
I considered adding a comment for that pointing out that it doesn't matter. The important thing is that the list shouldn't be empty. Now that I look at it again, even though the comment wouldn't hurt, it would actually be longer than reseting the front value to false :-)

http://reviews.llvm.org/D6497

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/