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