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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
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();
----------------
jmorse wrote:
> 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?
We're using an `IntervalMap<IndexT, char>`, so the unused mapped values are those chars. Every interval in the map does correspond to a non-empty range of set bits, yes, but writing `for (auto It : *Intervals.get())` doesn't work because:

```
llvm/include/llvm/ADT/CoalescingBitVector.h:95:21: error: member reference base type 'char' is not a structure or union
      Bits += 1 + It.stop() - It.start();
```


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:100
+  ///
+  /// This method does /not/ support setting a bit that has already been set,
+  /// for efficiency reasons. If possible, restructure your code to not set the
----------------
aprantl wrote:
> What happens if I set an already-set bit? Will the data structure get inconsistent, less efficient, crash? Should there be an assert(!test(Index)) here?
IntervalMap asserts if you try to make an overlapping insert. I've added the suggested assert.


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


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

https://reviews.llvm.org/D74984