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

[Yaron Keren via llvm-commits]

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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150918/39aead52/attachment.html>