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

Frederic Riss friss at apple.com
Mon Dec 8 12:23:00 PST 2014


================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:49
@@ +48,3 @@
+  if (SeenInstructionInRange.front())
+    SeenInstructionInRange.push_front(false);
+  Ranges.push_back(
----------------
dblaikie wrote:
> So 'SeenInstructionInRange' will just keep growing as the function grows? Should we try to limit it in any way.
> 
> Perhaps an alternative strategy would be to have a "currently unseen" std::shared_ptr<bool>, and then in "markOpenRangesNonEmpty" it would be:
> 
>   if (CurrentlySeen)
>     *CurrentlySeen = true;
>   CurrentlySeen = llvm::make_unique<bool>(false);
> 
> and then in "endInstrRange" the std::shared_ptr<bool> would be:
> 
>   if (*Ranges.back().NonEmpty) {
>     Ranges.back().second = &MI;
>     Ranges.back().NonEmpty = nullptr;
>   } ...
> 
> (would this make it any easier to move the NonEmpty std::shared_ptr<bool> to the InstrRanges rather than into its elements (so we just have one per range set, rather than one per range)? I forget)
> 
> In any case, hoping Alexey can weigh in, as the person who (re)wrote most of this code most recently.
Yes, SeenInstructionInRange only grows with the function. It will however never contain more items than there are ranges, thus the space complexity of the DbgValueHistoryCalculator stays the same.

Your suggestion would work, but I'm not sure it is easier to grok for the reader :-) Putting the status in the range set rather than in the range itself would require tracking how many open ranges are empty for this set, which shouldn't be an issue.

Piggy backing on your shared_ptr idea, here's another one requiring at most one shared_ptr<bool> at a time and with lifetime semantics a bit simpler than in your proposal: make NonEmpty a weak_ptr (and call it Empty). The value of the pointed bool doesn't matter, but the weak_ptr validity is the real information.

markOpenRanges becomes:
```
if (SeenInstructionInRange.use_count())  // Are there empty open ranges?
  SeenInstructionInRange = make_shared<bool>(true); // This invalidates all current weak_ptrs 
```

and in endInstrRange:
```
if (!Ranges.back().Empty.lock())
  Ranges.back().second = &MI;
else
  Ranges.pop_back();
```

There is some hidden complexity in the shared_ptr handling though (management of the shared_ptr use lists), that make me believe that the proposed solution is still more efficient. I don't think this is a big deal because these lists should be short, but I wouldn't be surprised if some pathological case shows up one day.

(and yes, Alexey's input would be much welcome)

http://reviews.llvm.org/D6497






More information about the llvm-commits mailing list