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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Thanks for this!

In D77027#1966204 <https://reviews.llvm.org/D77027#1966204>, @bmoody wrote:

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


Could you migrate this code to use just the operator in this change since the size check will now be redundant?

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

I think this code is probably fine - all the BitVectors start by being initialized from "emptyRegisters" (pre-initialized with the necessary size) in exegesis::Instruction::create, I think.

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

Is it tested? Might be worth leaving a FIXME of "hey, this is untested code".



================
Comment at: llvm/include/llvm/ADT/BitVector.h:542
-
-    // Verify that any extra words are all zeros.
-    if (i != ThisWords) {
----------------
Hmm - looking at this more closely (ie: I'm now actually looking at the code rather than just the commentary/discussion) this code makes me a bit uncomfortable - looks pretty intentionally designed to support potentially equal comparison of differently lengthed BitVectors. Is there any history of how/when this was added/potentially used?


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

https://reviews.llvm.org/D77027