[BitVector] Define BitWord as uintptr_t

[ekatz]

Summary

Define BitVector::BitWord as uintptr_t instead of unsigned long, as long does not necessarily translates to a pointer size (especially on 64-bit Visual Studio).

[lebedev.ri]

llvm/include/llvm/ADT/BitVector.h

74

It may be better to split NFC cleanup (rest of the patch) and this change into two separate patches.

[ekatz]

llvm/include/llvm/ADT/BitVector.h

74

It is no cleanup, but the same change. It used to be unsigned long, and therefore the numbers (changed) had the suffix UL (unsigned long). As there is no appropriate suffix for size_t, we need to apply casting instead.

[kparzysz]

There is no guarantee that size_t will match the size of a pointer either. Why do you want that anyway?

[lebedev.ri]

llvm/include/llvm/ADT/BitVector.h

74

I think the point i was making was missed.
The suffix->cast cleanup is valid regardless, it can be done beforehand,
and then this patch becomes a single line change.

[ekatz]

llvm/include/llvm/ADT/BitVector.h

74

OK, as there are already other places, in the current version, where the cast to BitWord is made, then this sounds reasonable. Is there a way to post 2 patches in a single review using Phabricator? Or should I open a separate review?

[ekatz]

In D69336#1719048, @kparzysz wrote:

There is no guarantee that size_t will match the size of a pointer either. Why do you want that anyway?

Correct. I will change it to uintptr_t.
This is needed for the heavy usage of countPopulation, countTrailingZeros and countLeadingZeros.

[lebedev.ri]

llvm/include/llvm/ADT/BitVector.h

74

Separate review (that new patch should be the first one, this D69336 should be based on it)

[kparzysz]

There are custom implementations of these functions for 64-bit unsigned values, so you should make it uint64_t. There is no connection with the pointer size.

[ekatz]

In D69336#1719215, @kparzysz wrote:

There are custom implementations of these functions for 64-bit unsigned values, so you should make it uint64_t. There is no connection with the pointer size.

Some intrinsics only available for specific architectures. For example, on Windows, _BitScanReverse64 is only available in a 64-bit build (refer to LeadingZerosCounter<T,8> in MathExtras.h).

[lattner]

This looks like progress to me. I personally would go with size_t even despite the (theoretical IMO) concern about size_t being a different size, but either way works for me, and both are better than ulong.

[ekatz]

I cannot commit this patch myself. Please commit it (and the parent change, of course) in my behalf.

Author: Ehud Katz <[e-mail]>

Thanks.

llvm/include/llvm/ADT/BitVector.h

public:
   bool operator!=(const const_set_bits_iterator_impl &Other) const {
     assert(&Parent == &Other.Parent &&
            "Comparing iterators from different BitVectors");
     return Current != Other.Current;
   }
 };
 
 class BitVector {
-  typedef unsigned long BitWord;
+  typedef uintptr_t BitWord;
 
   enum { BITWORD_SIZE = (unsigned)sizeof(BitWord) * CHAR_BIT };
 
   static_assert(BITWORD_SIZE == 64 || BITWORD_SIZE == 32,
                 "Unsupported word size");
 
   MutableArrayRef<BitWord> Bits; // Actual bits.
   unsigned Size;                 // Size of bitvector in bits.