[PATCH] D74984: [ADT] Add CoalescingBitVector, implemented using IntervalMap [1/3]
Vedant Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 24 13:16:13 PST 2020
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
More information about the llvm-commits
mailing list