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

558

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

558

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

[jtouton]

include/llvm/ADT/SmallBitVector.h

558

(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

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

.. = 1UL << i;

[jtouton]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

145

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

558

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

558

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

560

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 (NumBaseBits == 64 && MaskWords >= 2)
+      M |= uintptr_t(Mask[1]) << 32;
   }
 };
 
 inline SmallBitVector
 operator&(const SmallBitVector &LHS, const SmallBitVector &RHS) {
   SmallBitVector Result(LHS);
   Result &= RHS;
   return Result;

lib/Target/X86/X86ISelLowering.cpp

if (LoadSDNode *LD = dyn_cast<LoadSDNode>(SrcOp)) {
     }
 
     // (Offset % 16 or 32) must be multiple of 4. Then address is then
     // Ptr + (Offset & ~15).
     if (Offset < 0)
       return SDValue();
     if ((Offset % RequiredAlign) & 3)
       return SDValue();
-    int64_t StartOffset = Offset & ~(RequiredAlign-1);
+    int64_t StartOffset = Offset & ~int64_t(RequiredAlign-1);
     if (StartOffset) {
       SDLoc DL(Ptr);
       Ptr = DAG.getNode(ISD::ADD, DL, Ptr.getValueType(), Ptr,
                         DAG.getConstant(StartOffset, DL, Ptr.getValueType()));
     }
 
     int EltNo = (Offset - StartOffset) >> 2;
     unsigned NumElems = VT.getVectorNumElements();

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

void ThreadSanitizer::initializeCallbacks(Module &M) {
   IRBuilder<> IRB(M.getContext());
   // Initialize the callbacks.
   TsanFuncEntry = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
       "__tsan_func_entry", IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));
   TsanFuncExit = checkSanitizerInterfaceFunction(
       M.getOrInsertFunction("__tsan_func_exit", IRB.getVoidTy(), nullptr));
   OrdTy = IRB.getInt32Ty();
   for (size_t i = 0; i < kNumberOfAccessSizes; ++i) {
-    const size_t ByteSize = 1 << i;
+    const size_t ByteSize = size_t(1 << i);
     const size_t BitSize = ByteSize * 8;
     SmallString<32> ReadName("__tsan_read" + itostr(ByteSize));
     TsanRead[i] = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
         ReadName, IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));
 
     SmallString<32> WriteName("__tsan_write" + itostr(ByteSize));
     TsanWrite[i] = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
         WriteName, IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));

 
 bool ThreadSanitizer::instrumentAtomic(Instruction *I, const DataLayout &DL) {
   IRBuilder<> IRB(I);
   if (LoadInst *LI = dyn_cast<LoadInst>(I)) {
     Value *Addr = LI->getPointerOperand();
     int Idx = getMemoryAccessFuncIndex(Addr, DL);
     if (Idx < 0)
       return false;
-    const size_t ByteSize = 1 << Idx;
+    const size_t ByteSize = size_t(1 << Idx);
     const size_t BitSize = ByteSize * 8;
     Type *Ty = Type::getIntNTy(IRB.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                      createOrdering(&IRB, LI->getOrdering())};
     CallInst *C = CallInst::Create(TsanAtomicLoad[Idx], Args);
     ReplaceInstWithInst(I, C);
 
   } else if (StoreInst *SI = dyn_cast<StoreInst>(I)) {
     Value *Addr = SI->getPointerOperand();
     int Idx = getMemoryAccessFuncIndex(Addr, DL);
     if (Idx < 0)
       return false;
-    const size_t ByteSize = 1 << Idx;
+    const size_t ByteSize = size_t(1 << Idx);
     const size_t BitSize = ByteSize * 8;
     Type *Ty = Type::getIntNTy(IRB.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                      IRB.CreateIntCast(SI->getValueOperand(), Ty, false),
                      createOrdering(&IRB, SI->getOrdering())};
     CallInst *C = CallInst::Create(TsanAtomicStore[Idx], Args);
     ReplaceInstWithInst(I, C);
   } else if (AtomicRMWInst *RMWI = dyn_cast<AtomicRMWInst>(I)) {
     Value *Addr = RMWI->getPointerOperand();
     int Idx = getMemoryAccessFuncIndex(Addr, DL);
     if (Idx < 0)
       return false;
     Function *F = TsanAtomicRMW[RMWI->getOperation()][Idx];
     if (!F)
       return false;
-    const size_t ByteSize = 1 << Idx;
+    const size_t ByteSize = size_t(1 << Idx);
     const size_t BitSize = ByteSize * 8;
     Type *Ty = Type::getIntNTy(IRB.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                      IRB.CreateIntCast(RMWI->getValOperand(), Ty, false),
                      createOrdering(&IRB, RMWI->getOrdering())};
     CallInst *C = CallInst::Create(F, Args);
     ReplaceInstWithInst(I, C);
   } else if (AtomicCmpXchgInst *CASI = dyn_cast<AtomicCmpXchgInst>(I)) {
     Value *Addr = CASI->getPointerOperand();
     int Idx = getMemoryAccessFuncIndex(Addr, DL);
     if (Idx < 0)
       return false;
-    const size_t ByteSize = 1 << Idx;
+    const size_t ByteSize = size_t(1 << Idx);
     const size_t BitSize = ByteSize * 8;
     Type *Ty = Type::getIntNTy(IRB.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                      IRB.CreateIntCast(CASI->getCompareOperand(), Ty, false),
                      IRB.CreateIntCast(CASI->getNewValOperand(), Ty, false),
                      createOrdering(&IRB, CASI->getSuccessOrdering()),
                      createOrdering(&IRB, CASI->getFailureOrdering())};