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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Nice, I can think of a few other places where this could prove useful (the register coalescing code for variable locations for starters). Some comments inline, plus

- `|=` / set union could do with being covered in the unittests too,
- Maybe some allocations could be avoided by using things like IntervalMap iterator setStartUnchecked and similar, but this looks good regardless.

It looks like setting already-set bits and resetting not-set bits isn't a supported operation. `set` indicates not to do that in the doc comment, `reset` even goes as far as to assert-fail if the wrong interval is looked up. These are two things that, without any additional context, I would expect something acting like a bitvector to support, it's quite a common operation on flags integers for example, and these things are supported by BitVector and SparseBitVector. Are there technical reasons for these limits, or is it to discourage CoalescingBitVector from being used in scenarios where it won't be useful, or otherwise?

I see that the implementation of `operator|=` follows the same code path as plain `set`, i.e. `insert(a, b)`, making it possible that `operator|=` will set already-set bits. Is that a supported operation for `operator|=` but not for `set`, or am I missing something?



================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:30
+/// performance for non-sequential find() operations.
+template <typename EltT, unsigned N = 16> class CoalescingBitVector {
+  using ThisT = CoalescingBitVector<EltT, N>;
----------------
Could we get a

      static_assert(std::is_integral<EltT>::value, "Key must be an integer type.");

Plus documentation of the EltT type parameter -- if I understand correctly, it's the storage type for bitvector elements, that's used as the domain type for the interval map?


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:55-59
+  ThisT &operator=(const ThisT &RHS) {
+    clear();
+    initFromRHS(RHS);
+    return *this;
+  }
----------------
I'm not familiar with LLVMs approach to allocators, but is it alright that the RHS allocator isn't carried over into `this`? This happens for the rvalue assignment operator below.


================
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();
----------------
Nit, can this be a range-based loop?


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:231-232
+
+      assert(OffsetIntoMapIterator == 0 && "Not implemented");
+      assert(Index <= CachedStop && "Cannot advance to OOB index");
+      if (Index < CachedStart)
----------------
Just so that I understand: advanceTo is aiming for a small set of use cases (i.e.: just `find`), and these assertions are to discourage anyone from using advanceTo for other purposes?


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

https://reviews.llvm.org/D74984