[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:55-59
+  ThisT &operator=(const ThisT &RHS) {
+    clear();
+    initFromRHS(RHS);
+    return *this;
+  }
----------------
jmorse wrote:
> 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.
initFromRHS does a copy into a pre-existing IntervalMap, which has already been initialized with this->Alloc. I don't think changing the Alloc would be correct, as the lifetime of RHS.Alloc may end before the lifetime of this->Alloc.


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:79
+  /// Count the number of set bits.
+  unsigned count() const {
+    unsigned Bits = 0;
----------------
aprantl wrote:
> Is this consistent with how the other bitvector types are naming this function? Then that's fine. In DenseSet, count(Elt) is a contains(Elt) function and I don't know if we want to be consistent with that.
Yeah, SparseBitVector::count() also counts the number of set bits.


================
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:
> 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.


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:82
+    for (auto It = Intervals->begin(), End = Intervals->end(); It != End; ++It)
+      Bits += 1 + It.stop() - It.start();
+    return Bits;
----------------
aprantl wrote:
> So empty ranges are disallowed? Should we assert that? Or is that too slow?
There isn't a way to construct an empty interval, AFAICT.


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:142
+
+  /// Reset all bits present in \p RHS.
+  bool intersectWithComplement(const ThisT &RHS) {
----------------
aprantl wrote:
> document \return
It looks like the return value from intersectWithComplement is generally ignored, so I've simply removed it.


================
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)
----------------
jmorse wrote:
> 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?
Yes, exactly. Actually the `OffsetIntoMapIterator == kIteratorAtTheEndOffset` check is dead code, let me remove that.


================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:297
+  void print(raw_ostream &OS) const {
+    // LLDB swallows the first line of output after callling dump(). Add
+    // newlines before/after the braces to work around this.
----------------
aprantl wrote:
> Really? If anything should this be part of the dump method instead?
Yeah, while debugging this `p BV.dump()` would result in:

```
(lldb) p BV.dump()
(lldb)1]}
```

or something. Incidentally, the same thing happens when doing `p SM.dump(SourceLoc)` in clang. I filed llvm.org/PR45011.


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

https://reviews.llvm.org/D74984