[PATCH] D74984: [ADT] Add CoalescingBitVector, implemented using IntervalMap [1/3]

Adrian Prantl via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 27 08:27:27 PST 2020


aprantl added inline comments.


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:185
+          ++CurrentOlap;
+          continue;
+        }
----------------
vsk wrote:
> jmorse wrote:
> > As this only considers one overlap at a time, won't this run into difficulties with two overlaps in one RHS interval? Such as:
> > 
> >   RHS:       ---------------------------------
> >   LHS:           --------         ---------
> > 
> > If I flip the final Union test in the unit tests:
> > 
> >   unionIs({2, 3, 4, 6, 7}, {1, 2, 3, 4, 5, 6, 7, 8}, {1, 2, 3, 4, 5, 6, 7, 8});
> > 
> > It coughs up an "Overlapping insert" assertion failure for me. Plus, if the second overlap is skipped, it would block any other overlap being considered, because we would never see the RHS interval it overlaps with and make progress.
> > 
> > (Maybe something less powerful/expressive than set union would be sufficient, if this proves painful to implement).
> You're right, we'd need to consider multiple overlaps per interval borrowed from RHS. I've gone ahead and just fixed this for posterity (I introduced a `getNonOverlappingParts` helper to do this), but was/remain very tempted to just delete the operator|= implementation.
> 
> As LiveDebugValues doesn't require a general set union operation, it's tempting to just punt on this, but it feels like that would be cheating others who may be interested in evaluating the new container.
> but it feels like that would be cheating others who may be interested in evaluating the new container.

I think that when we're adding a new data structure to ADT it should be generally useful even if some of the functionality is only use in unit tests.


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

https://reviews.llvm.org/D74984





More information about the llvm-commits mailing list