Fixed Visual Studio warnings.

[jtouton]

Summary

Simplified applyMask by consolidating common code for 32-bit and 64-bit builds.

Extend mask value to 64 bits before taking its complement.

Made 64-bit conversions explicit.

[yaron.keren]

In what configuration: VC, target do you see the warnings?

[ismail]

In D11890#220197, @yaron.keren wrote:

In what configuration: VC, target do you see the warnings?

VS2015 produces those for me.

[jtouton]

Correct, VS2015 building 64-bit binaries.

[yaron.keren]

include/llvm/ADT/SmallBitVector.h

563

What happens when MaskWords is 2 and we're running on a system where uintptr_t is 32 bits?

[jtouton]

include/llvm/ADT/SmallBitVector.h

563

The upper word is ignored (just like before this change).

[jtouton]

include/llvm/ADT/SmallBitVector.h

563

(Note that NumBaseBits is defined based on sizeof(uintptr_t), line 39 above. It's a host value, not a target value.)

[kcc]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

145 ↗

(On Diff #31626)

Wouldn't it be better (more readable to write like this? (Here and below)

.. = 1UL << i;

[jtouton]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

145 ↗

(On Diff #31626)

UL on MSVC is always 32-bit, so you'd get the same warning. Another alternative would be:

size_t(1) << i

...but I can't imagine a universe where i would be large enough for that to actually matter.

[kcc]

I am a big fan of compiler warnings but this one (1 << i) sounds bogus.
Isn't there a way to represent a size_t value 1 w/o wirting (size_t)1?

(size_t)1 << i
in this case is clearly much better than
(size_t)(1 << i)

[yaron.keren]

Committed the X86ISelLowering.cpp patch in r245161.

[yaron.keren]

Fixed several ThreadSanitizer.cpp issues in r245167. I don't have VS2015 config to test the warning and the online compiler is out but

const unsigned ByteSize = 1U << i;

should be OK. Please update if it's still warning.

[yaron.keren]

include/llvm/ADT/SmallBitVector.h

563

Silently ignore isn't good (problem with the original code, not your patch). At least

assert((NumBaseBits == 64 || MaskWords == 1) && "Mask is larger than base!");

otherwise, LGTM.

[jtouton]

• Fixed Visual Studio warning.
• Added assert in applyMask to verify that an appropriate value was passed for MaskWords.

[jtouton]

Sorry for the long wait; my mail filters ate notifications about updates to this diff, and I'm very new to Phabricator.

[yaron.keren]

include/llvm/ADT/SmallBitVector.h

563

Remove the inner parenthesis. These were required in the original assert due to || but with all operators being && they are no longer needed.

565

The assert is not indented while the brace too much, clang-format the whole function.

[jtouton]

• Fixed Visual Studio warning.
• Added assert in applyMask to verify that an appropriate value was passed for MaskWords.
• Fixed formatting issues.

[jtouton]

• Fixed formatting issues.

[yaron.keren]

The asserts on unit tests, BitVectorTest/1.PortableBitMask that calls

TypeParam A;
const uint32_t Mask1[] = { 0x80000000, 6, 5 };

A.resize(10);
A.setBitsInMask(Mask1, 3);

Now setBitsInMask is documented:

/// setBitsInMask - Add '1' bits from Mask to this vector. Don't resize.
/// This computes "*this |= Mask".

and it can not | a small bit vector with a 3x32 bit mask without resizing. It is an error which should not go silent and the test should be modified.

Jakob, do we miss something?

[jtouton]

Is there anything more I need to do for this one?

[yaron.keren]

r247972.

include/llvm/ADT/SmallBitVector.h

if (isSmall())
       applyMask<false, true>(Mask, MaskWords);
     else
       getPointer()->clearBitsNotInMask(Mask, MaskWords);
   }
 
 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;
+    }
 };
 
 inline SmallBitVector
 operator&(const SmallBitVector &LHS, const SmallBitVector &RHS) {
   SmallBitVector Result(LHS);
   Result &= RHS;
   return Result;
 }