Page MenuHomePhabricator

Fixed Visual Studio warnings.
ClosedPublic

Authored by jtouton on Aug 9 2015, 5:24 PM.

Details

Reviewers
yaron.keren
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.

Diff Detail

Event Timeline

jtouton updated this revision to Diff 31626.Aug 9 2015, 5:24 PM
jtouton retitled this revision from to Fixed Visual Studio warnings..
jtouton updated this object.
jtouton added a subscriber: llvm-commits.

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

ismail added a subscriber: ismail.Aug 10 2015, 12:03 AM

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

VS2015 produces those for me.

Correct, VS2015 building 64-bit binaries.

yaron.keren added inline comments.Aug 10 2015, 9:48 AM
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 added inline comments.Aug 10 2015, 11:01 AM
include/llvm/ADT/SmallBitVector.h
563

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

jtouton added inline comments.Aug 10 2015, 11:06 AM
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 added a subscriber: kcc.Aug 10 2015, 1:38 PM
kcc added inline comments.
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 added inline comments.Aug 10 2015, 1:42 PM
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 added a comment.Aug 12 2015, 3:21 PM

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)

Committed the X86ISelLowering.cpp patch in r245161.

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 added inline comments.Aug 15 2015, 12:40 PM
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 updated this revision to Diff 34014.Sep 3 2015, 10:43 PM
  • Fixed Visual Studio warning.
  • Added assert in applyMask to verify that an appropriate value was passed for MaskWords.

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

yaron.keren added inline comments.Sep 4 2015, 4:56 AM
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 updated this revision to Diff 34116.Sep 5 2015, 5:06 PM
  • Fixed Visual Studio warning.
  • Added assert in applyMask to verify that an appropriate value was passed for MaskWords.
  • Fixed formatting issues.
jtouton updated this revision to Diff 34167.Sep 7 2015, 10:21 AM
  • Fixed formatting issues.

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?

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

yaron.keren accepted this revision.Sep 17 2015, 11:36 PM
yaron.keren added a reviewer: yaron.keren.
This revision is now accepted and ready to land.Sep 17 2015, 11:36 PM