[PATCH] D77027: Make BitVector::operator== return false for different-sized vectors

[Brad Moody via Phabricator via llvm-commits]

bmoody added a comment.

I've reviewed all the uses of BitVector::operator==/!= in llvm and clang by renaming the functions and checking all the code that failed to compile.  As far as I can tell, all uses should be either unaffected by the change, or will be more correct with the change applied.

---

llvm/include/llvm/Analysis/TargetLibraryInfo.h:

  bool areInlineCompatible(const TargetLibraryInfo &CalleeTLI,
                           bool AllowCallerSuperset) const {
    if (!AllowCallerSuperset)
      return OverrideAsUnavailable == CalleeTLI.OverrideAsUnavailable;
    BitVector B = OverrideAsUnavailable;
    B |= CalleeTLI.OverrideAsUnavailable;
    // We can inline if the union of the caller and callee's nobuiltin
    // attributes is no stricter than the caller's nobuiltin attributes.
    return B == OverrideAsUnavailable;
  }

Not affected by the proposed change - all vectors have constant size NumLibFuncs.

---

llvm/lib/CodeGen/RegisterClassInfo.cpp:

  const BitVector &RR = MF->getRegInfo().getReservedRegs();
  if (Reserved.size() != RR.size() || RR != Reserved) {
    Update = true;
    Reserved = RR;
  }

Not affected by the proposed change - code has an explicit size check.

---

llvm/lib/Transforms/Coroutines/CoroFrame.cpp:

  Changed |= (S.Kills != SavedKills) || (S.Consumes != SavedConsumes);
  
  if (S.Kills != SavedKills) {
    LLVM_DEBUG(dbgs() << "\nblock " << I << " follower " << SI->getName()
                      << "\n");
    LLVM_DEBUG(dump("S.Kills", S.Kills));
    LLVM_DEBUG(dump("SavedKills", SavedKills));
  }
  if (S.Consumes != SavedConsumes) {
    LLVM_DEBUG(dbgs() << "\nblock " << I << " follower " << SI << "\n");
    LLVM_DEBUG(dump("S.Consume", S.Consumes));
    LLVM_DEBUG(dump("SavedCons", SavedConsumes));
  }

Not affected by the proposed change - all the vectors in this code have the same size. They're initialised above, in the same function:

  // Initialize every block so that it consumes itself
  for (size_t I = 0; I < N; ++I) {
    auto &B = Block[I];
    B.Consumes.resize(N);
    B.Kills.resize(N);
    B.Consumes.set(I);
  }



---

llvm/tools/llvm-exegesis/lib/MCInstrDescView.cpp:

  const BitVector *BitVectorCache::getUnique(BitVector &&BV) const {
    for (const auto &Entry : Cache)
      if (*Entry == BV)
        return Entry.get();
    Cache.push_back(std::make_unique<BitVector>());
    auto &Entry = Cache.back();
    Entry->swap(BV);
    return Entry.get();
  }

Looks like the proposed change will fix a (probably latent) bug in this code. Currently it seems like this function could return a pointer to a BitVector with a different size than the one that was passed in, which would be surprising.

---

llvm/include/llvm/ADT/PackedVector.h:

  bool operator==(const PackedVector &RHS) const {
    return Bits == RHS.Bits;
  }
  
  bool operator!=(const PackedVector &RHS) const {
    return Bits != RHS.Bits;
  }

I think this is another latent bug.  PackedVector::operator== doesn't appear to be used anywhere.


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

https://reviews.llvm.org/D77027