[llvm] r247972 - Simplify SmallBitVector::applyMask by consolidating common code for 32-bit and 64-bit builds.

Aaron Ballman via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 18 09:16:08 PDT 2015


Thank you for taking care of this!

~Aaron

On Fri, Sep 18, 2015 at 12:03 PM, Yaron Keren <yaron.keren at gmail.com> wrote:
> r247993
>
> Compiled without warnings and passed tests on VC 2013 32 bit and 64 bit.
>
> 2015-09-18 16:13 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>>
>> On Fri, Sep 18, 2015 at 9:10 AM, Yaron Keren <yaron.keren at gmail.com>
>> wrote:
>> > I think this will work OK, will test:
>> >
>> >   void applyMask(const uint32_t *Mask, unsigned MaskWords) {
>> >     assert(MaskWords <= sizeof(uintptr_t) && "Mask is larger than
>> > base!");
>> >     uintptr_t M = Mask[0];
>> >     if (NumBaseBits == 64)
>> >       M |= uint64_t(Mask[1]) << 32;
>> >     if (InvertMask) M = ~M;
>> >     if (AddBits) setSmallBits(getSmallBits() | M);
>> >     else         setSmallBits(getSmallBits() & ~M);
>> >   }
>>
>> Formatting aside, that looks reasonable to me. ;-) Thank you!
>>
>> ~Aaron
>>
>> >
>> >
>> >
>> > 2015-09-18 16:02 GMT+03:00 Yaron Keren <yaron.keren at gmail.com>:
>> >>
>> >> The condition is on sizeof(uintptr_t) * CHAR_BIT which is a compile
>> >> time
>> >> constant. If it's not possible to make the same code work for both 32
>> >> and 64
>> >> bits without warnings it could be done with #ifdef (ugly) or template
>> >> specialization for 32 and 64 bits where the unrequired version will not
>> >> be
>> >> instantiated.
>> >>
>> >>
>> >> 2015-09-18 15:40 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>> >>>
>> >>> On Fri, Sep 18, 2015 at 8:37 AM, Yaron Keren <yaron.keren at gmail.com>
>> >>> wrote:
>> >>> > Thanks, this code will not be reached in 32 bits as the assert will
>> >>> > fail but
>> >>> > that won't hold in release config, the shift code should probably be
>> >>> > in
>> >>> > a
>> >>> > conditional of both NumBaseBits and MaskWords, as was the original
>> >>> > code. It
>> >>> > should be still possible to merge them but more carefully.
>> >>>
>> >>> In theory, the code doesn't have to be reached for UB to cause
>> >>> problems anyway. I wasn't certain that upcasting to 64-bits was the
>> >>> right approach either, that's why I punted on just fixing it myself.
>> >>> It still seems like it is solvable though.
>> >>>
>> >>> ~Aaron
>> >>>
>> >>> >
>> >>> >
>> >>> > 2015-09-18 15:20 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>> >>> >>
>> >>> >> On Fri, Sep 18, 2015 at 2:35 AM, Yaron Keren via llvm-commits
>> >>> >> <llvm-commits at lists.llvm.org> wrote:
>> >>> >> > Author: yrnkrn
>> >>> >> > Date: Fri Sep 18 01:35:12 2015
>> >>> >> > New Revision: 247972
>> >>> >> >
>> >>> >> > URL: http://llvm.org/viewvc/llvm-project?rev=247972&view=rev
>> >>> >> > Log:
>> >>> >> > Simplify SmallBitVector::applyMask by consolidating common code
>> >>> >> > for
>> >>> >> > 32-bit and 64-bit builds.
>> >>> >> > Extend mask value to 64 bits before taking its complement and
>> >>> >> > assert
>> >>> >> > when mask is
>> >>> >> > too large to apply in the small case (previously the extra words
>> >>> >> > were
>> >>> >> > silently ignored).
>> >>> >> >
>> >>> >> > http://reviews.llvm.org/D11890
>> >>> >> >
>> >>> >> > Patch by James Touton!
>> >>> >> >
>> >>> >> >
>> >>> >> > Modified:
>> >>> >> >     llvm/trunk/include/llvm/ADT/SmallBitVector.h
>> >>> >> >     llvm/trunk/unittests/ADT/BitVectorTest.cpp
>> >>> >> >
>> >>> >> > Modified: llvm/trunk/include/llvm/ADT/SmallBitVector.h
>> >>> >> > URL:
>> >>> >> >
>> >>> >> >
>> >>> >> > http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallBitVector.h?rev=247972&r1=247971&r2=247972&view=diff
>> >>> >> >
>> >>> >> >
>> >>> >> >
>> >>> >> > ==============================================================================
>> >>> >> > --- llvm/trunk/include/llvm/ADT/SmallBitVector.h (original)
>> >>> >> > +++ llvm/trunk/include/llvm/ADT/SmallBitVector.h Fri Sep 18
>> >>> >> > 01:35:12
>> >>> >> > 2015
>> >>> >> > @@ -553,17 +553,15 @@ public:
>> >>> >> >  private:
>> >>> >> >    template<bool AddBits, bool InvertMask>
>> >>> >> >    void applyMask(const uint32_t *Mask, unsigned MaskWords) {
>> >>> >> > -    if (NumBaseBits == 64 && MaskWords >= 2) {
>> >>> >> > -      uint64_t M = Mask[0] | (uint64_t(Mask[1]) << 32);
>> >>> >> > -      if (InvertMask) M = ~M;
>> >>> >> > -      if (AddBits) setSmallBits(getSmallBits() | M);
>> >>> >> > -      else         setSmallBits(getSmallBits() & ~M);
>> >>> >> > -    } else {
>> >>> >> > -      uint32_t M = Mask[0];
>> >>> >> > -      if (InvertMask) M = ~M;
>> >>> >> > -      if (AddBits) setSmallBits(getSmallBits() | M);
>> >>> >> > -      else         setSmallBits(getSmallBits() & ~M);
>> >>> >> > +    uintptr_t M = Mask[0];
>> >>> >> > +    if (MaskWords != 1) {
>> >>> >> > +      assert(NumBaseBits == 64 && MaskWords == 2 &&
>> >>> >> > +             "Mask is larger than base!");
>> >>> >> > +      M |= uintptr_t(Mask[1]) << 32;
>> >>> >>
>> >>> >> This patch introduces undefined behavior on platforms where
>> >>> >> uintptr_t
>> >>> >> is 32-bits, such as on Windows when compiling with MSVC in 23-bit
>> >>> >> mode.
>> >>> >>
>> >>> >> 67>E:\llvm\llvm\include\llvm/ADT/SmallBitVector.h(560): warning
>> >>> >> C4293:
>> >>> >> '<<': shift count negative or too big, undefined behavior
>> >>> >> (compiling
>> >>> >> source file
>> >>> >>
>> >>> >> E:\llvm\llvm\tools\clang\lib\StaticAnalyzer\Core\FunctionSummary.cpp)
>> >>> >> 67>  E:\llvm\llvm\include\llvm/ADT/SmallBitVector.h(521): note: see
>> >>> >> reference to function template instantiation 'void
>> >>> >> llvm::SmallBitVector::applyMask<true,false>(const uint32_t
>> >>> >> *,unsigned
>> >>> >> int)' being compiled (compiling source file
>> >>> >>
>> >>> >> E:\llvm\llvm\tools\clang\lib\StaticAnalyzer\Core\FunctionSummary.cpp)
>> >>> >>
>> >>> >> I am reverting the patch (and r247972) temporarily since this also
>> >>> >> generates a lot of warnings (everywhere SmallBitVector.h ends up
>> >>> >> being
>> >>> >> included from) in r247983.
>> >>> >>
>> >>> >> ~Aaron
>> >>> >>
>> >>> >> >      }
>> >>> >> > +    if (InvertMask) M = ~M;
>> >>> >> > +    if (AddBits) setSmallBits(getSmallBits() | M);
>> >>> >> > +    else         setSmallBits(getSmallBits() & ~M);
>> >>> >> >    }
>> >>> >> >  };
>> >>> >> >
>> >>> >> >
>> >>> >> > Modified: llvm/trunk/unittests/ADT/BitVectorTest.cpp
>> >>> >> > URL:
>> >>> >> >
>> >>> >> >
>> >>> >> > http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/BitVectorTest.cpp?rev=247972&r1=247971&r2=247972&view=diff
>> >>> >> >
>> >>> >> >
>> >>> >> >
>> >>> >> > ==============================================================================
>> >>> >> > --- llvm/trunk/unittests/ADT/BitVectorTest.cpp (original)
>> >>> >> > +++ llvm/trunk/unittests/ADT/BitVectorTest.cpp Fri Sep 18
>> >>> >> > 01:35:12
>> >>> >> > 2015
>> >>> >> > @@ -235,12 +235,12 @@ TYPED_TEST(BitVectorTest, PortableBitMas
>> >>> >> >    const uint32_t Mask1[] = { 0x80000000, 6, 5 };
>> >>> >> >
>> >>> >> >    A.resize(10);
>> >>> >> > -  A.setBitsInMask(Mask1, 3);
>> >>> >> > +  A.setBitsInMask(Mask1, 2);
>> >>> >> >    EXPECT_EQ(10u, A.size());
>> >>> >> >    EXPECT_FALSE(A.test(0));
>> >>> >> >
>> >>> >> >    A.resize(32);
>> >>> >> > -  A.setBitsInMask(Mask1, 3);
>> >>> >> > +  A.setBitsInMask(Mask1, 2);
>> >>> >> >    EXPECT_FALSE(A.test(0));
>> >>> >> >    EXPECT_TRUE(A.test(31));
>> >>> >> >    EXPECT_EQ(1u, A.count());
>> >>> >> >
>> >>> >> >
>> >>> >> > _______________________________________________
>> >>> >> > llvm-commits mailing list
>> >>> >> > llvm-commits at lists.llvm.org
>> >>> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> >>> >
>> >>> >
>> >>
>> >>
>> >
>
>


More information about the llvm-commits mailing list