[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