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

Alexey Samsonov vonosmas at gmail.com
Tue Dec 9 15:30:48 PST 2014


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.

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

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

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:206
@@ +205,3 @@
+
+        if (!MI.isPosition() && !MI.isImplicitDef() && !MI.isKill())
+          Result.markOpenRangesNonEmpty();
----------------
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?

================
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(); }
----------------
Note that this can leave your `SeenInstructionInRange` list have a single "true" value. This would probably still work, though.

http://reviews.llvm.org/D6497






More information about the llvm-commits mailing list