[PATCH] D107554: [DWARF][Verifier] Do not add child DieRangeInfo with empty address range to the parent.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.



>> I mean having insert look something like this:
>>
>>   bool DWARFVerifier::DieRangeInfo::intersects(const DieRangeInfo &OriginalRHS) const {
>>     bool Swap = Ranges.size() > RHS.Ranges.size();
>>     auto &LHS = Swap ? OriginalRHS : *this;
>>     auto &RHS = Swap ? *this : OriginalRHS;
>>     auto End = LHS.end();
>>     auto Iter = LHS.begin();
>>     while (Iter != End) {
>>       bool Intersects;
>>       iterator IntersectingIterator;
>>       std::tie(Intersects, IntersectingIterator) = Iter->intersects(RHS); // imagine a DieRangeInfo::intersects that returns both the bool, but also an iterator about where the intersection was
>>       if (Intersects)
>>         return Swap ? IntersectingIterator : Iter;
>>       ++Iter;
>>     }
>>     Children.insert(OriginalRHS);
>>     return Children.end();
>>   }
>>
>> This way, not only does the empty case perform better (if either LHS or RHS is empty, the algorithm will be quick - because the while loop would never run) - but also non-empty cases would be more efficient, I think, maybe?
>>
>> Though honestly - maybe a broader change in direction would be better: What if the DieRangeInfos were sorted (I guess they are, since they're in a std::set?)? Then it wouldn't be an exhaustive search, but use find to `find` the relevant part of the list to look at instead. Hmm, maybe sorted in a contiguous data structure (like std::vector) and then binary searching the flat structure - and also then being able to binary search only the remainder of the list when looking for the next entry? Not sure, though.
>
> I see. So you are speaking not about DieRangeInfo::intersects(const DieRangeInfo &RHS) but about DieRangeInfo::insert(const DieRangeInfo &RI).

Ah, sorry, no - I did mean insert, got the function name wrong when I was assembling the example code. (the implementation's still the implementation of insert - since it has the Children.insert code, etc, just the function name is incorrect)

> In suggested implementation we still need to check for OriginalRHS.Ranges.empty() to avoid storing it into children list:
>
>   if ( !OriginalRHS.Ranges.empty() ) {
>     Children.insert(OriginalRHS);
>     return Children.end();
>   }

That's fair. Is that causing scaling problems? In terms of making 'Children' large and so it's really slow to iterate through? Fair enough - yeah, then I don't object so much to just checking that more up-front (but maybe up-front inside "insert" rather than in the caller - to make "insert" more generically robust?

> But I understood your suggestion and will add binary search inside Children for DieRangeInfo::insert(const DieRangeInfo &RI).

There might still be room for this ^ work too, but probably in a separate patch/in addition, not as an alternative to the patch you're proposing here - I think this patch here can move forward, just with the check moved into "insert" so it's more generically robust.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107554/new/

https://reviews.llvm.org/D107554