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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Looking good; I now see that IntervalMap will fire an assertion if an overlapping insert is made, I'd previously thought it would just be inefficient, this explains why CoalescingBitVector can't do it.



================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:81
+    unsigned Bits = 0;
+    for (auto It = Intervals->begin(), End = Intervals->end(); It != End; ++It)
+      Bits += 1 + It.stop() - It.start();
----------------
vsk wrote:
> jmorse wrote:
> > Nit, can this be a range-based loop?
> I don't think so, as we'd be iterating over the unused mapped values in that case.
I wonder if I'm missing something here -- what are the unused mapped values? Isn't everything in the IntervalMap mapped to a value of zero, and so any interval that's present must correspond to an interval of set bits?


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:185
+          ++CurrentOlap;
+          continue;
+        }
----------------
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).


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

https://reviews.llvm.org/D74984