Hi, Doing some profiling of llc, I noticed that some bitvector operations took longer than usual. Then I noticed that too many copies of BitVector obejcts are created, even when such operations like &=, ^=, |= are performed on those bit vectors. I looked at the BitVector ADT implementation in BitVector.h and figured out that all assignment operations (except the usual assignment operator) return a copy of the bit vector, instead of a reference! I guess it was just overlooked. Below is the patch to fix it. Is it OK to commit? - Roman Index: BitVector.h ==================================================================--- BitVector.h (revision 62258) +++ BitVector.h (working copy) @@ -286,7 +286,7 @@ } // Intersection, union, disjoint union. - BitVector operator&=(const BitVector &RHS) { + BitVector &operator&=(const BitVector &RHS) { unsigned ThisWords = NumBitWords(size()); unsigned RHSWords = NumBitWords(RHS.size()); unsigned i; @@ -302,14 +302,14 @@ return *this; } - BitVector operator|=(const BitVector &RHS) { + BitVector &operator|=(const BitVector &RHS) { assert(Size == RHS.Size && "Illegal operation!"); for (unsigned i = 0; i < NumBitWords(size()); ++i) Bits[i] |= RHS.Bits[i]; return *this; } - BitVector operator^=(const BitVector &RHS) { + BitVector &operator^=(const BitVector &RHS) { assert(Size == RHS.Size && "Illegal operation!"); for (unsigned i = 0; i < NumBitWords(size()); ++i) Bits[i] ^= RHS.Bits[i];
On Jan 23, 1:51 pm, Roman Levenstein <romix.l... at googlemail.com> wrote:> Hi, > > Doing some profiling of llc, I noticed that some bitvector operations > took longer than usual. Then I noticed that too many copies of > BitVector obejcts are created, even when such operations like &=, ^=, > |= are performed on those bit vectors. > > I looked at the BitVector ADT implementation in BitVector.h and > figured out that all assignment operations (except the usual > assignment operator) return a copy of the bit vector, instead of a > reference! > I guess it was just overlooked. > > Below is the patch to fix it. Is it OK to commit?Yes. Please note in the commit message that the old semantics probably did not meet the expectations. With your patch, chained assignments happen to the right object. A very good catch, and a nice demonstration of how C++'s performance characteristics can be spoiled by small bugs like these. Cheers, Gabor> > - Roman > > Index: BitVector.h > ==================================================================> --- BitVector.h (revision 62258) > +++ BitVector.h (working copy) > @@ -286,7 +286,7 @@ > } > > // Intersection, union, disjoint union. > - BitVector operator&=(const BitVector &RHS) { > + BitVector &operator&=(const BitVector &RHS) { > unsigned ThisWords = NumBitWords(size()); > unsigned RHSWords = NumBitWords(RHS.size()); > unsigned i; > @@ -302,14 +302,14 @@ > return *this; > } > > - BitVector operator|=(const BitVector &RHS) { > + BitVector &operator|=(const BitVector &RHS) { > assert(Size == RHS.Size && "Illegal operation!"); > for (unsigned i = 0; i < NumBitWords(size()); ++i) > Bits[i] |= RHS.Bits[i]; > return *this; > } > > - BitVector operator^=(const BitVector &RHS) { > + BitVector &operator^=(const BitVector &RHS) { > assert(Size == RHS.Size && "Illegal operation!"); > for (unsigned i = 0; i < NumBitWords(size()); ++i) > Bits[i] ^= RHS.Bits[i]; > _______________________________________________ > LLVM Developers mailing list > LLVM... at cs.uiuc.edu http://llvm.cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Jan 23, 2009, at 6:43 AM, heisenbug wrote:> On Jan 23, 1:51 pm, Roman Levenstein <romix.l... at googlemail.com> > wrote: >> Hi, >> >> Doing some profiling of llc, I noticed that some bitvector operations >> took longer than usual. Then I noticed that too many copies of >> BitVector obejcts are created, even when such operations like &=, ^=, >> |= are performed on those bit vectors. >> >> I looked at the BitVector ADT implementation in BitVector.h and >> figured out that all assignment operations (except the usual >> assignment operator) return a copy of the bit vector, instead of a >> reference! >> I guess it was just overlooked. >> >> Below is the patch to fix it. Is it OK to commit? > > Yes. Please note in the commit message that the old > semantics probably did not meet the expectations. > With your patch, chained assignments happen to the right > object.Yes, please apply!> A very good catch, and a nice demonstration of how > C++'s performance characteristics can be spoiled > by small bugs like these.Too much subtlety :( -Chris
Seemingly Similar Threads
- [LLVMdev] Small problem in BitVector.h
- Interest in fast BitVector?
- [LLVMdev] [PATCH]: Add SparseBitmap implementation
- [LLVMdev] Default behavior of DeadMachineInstructionElim deletes all instructions
- [LLVMdev] Default behavior of DeadMachineInstructionElim deletes all instructions