[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 06:13:25 PDT 2015
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