Page MenuHomePhabricator

[BitVector] Define BitWord as uintptr_t
ClosedPublic

Authored by ekatz on Oct 23 2019, 5:18 AM.

Details

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).

Diff Detail

Event Timeline

ekatz created this revision.Oct 23 2019, 5:18 AM
lebedev.ri added inline comments.
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 marked an inline comment as done.Oct 23 2019, 10:15 AM
ekatz added inline comments.
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.

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

lebedev.ri added inline comments.Oct 23 2019, 10:29 AM
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 marked an inline comment as done.Oct 23 2019, 11:29 AM
ekatz added inline comments.
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?

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 added inline comments.Oct 23 2019, 12:02 PM
llvm/include/llvm/ADT/BitVector.h
74

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

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 added a comment.Oct 23 2019, 2:19 PM

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).

ekatz updated this revision to Diff 226184.Oct 23 2019, 2:34 PM
ekatz retitled this revision from [Support] Make BitVector::BitWord size_t to [BitVector] Define BitWord as uintptr_t.
ekatz edited the summary of this revision. (Show Details)
lattner accepted this revision.Oct 24 2019, 8:51 AM

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.

This revision is now accepted and ready to land.Oct 24 2019, 8:51 AM
ekatz added a comment.Oct 28 2019, 3:03 PM

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

Author: Ehud Katz <ehudkatz@gmail.com>

Thanks.

This revision was automatically updated to reflect the committed changes.