[PATCH] D50876: Clean up newly created header
mclow.lists closed this revision. mclow.lists added a comment. Landed as r340049 https://reviews.llvm.org/D50876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
mclow.lists added a comment. In https://reviews.llvm.org/D50876#1204531, @mclow.lists wrote: > @craig.topper - that's existing code; I'm not changing it. > If we have a test bot that I can test this against, I'm happy to update it. I'm not really sure that this code is actually used anywhere. https://reviews.llvm.org/D50876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
mclow.lists added a comment. @craig.topper - that's existing code; I'm not changing it. If we have a test bot that I can test this against, I'm happy to update it. https://reviews.llvm.org/D50876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
craig.topper added inline comments. Comment at: include/bit:140 static_assert(sizeof(unsigned) == 4, ""); return __popcnt(__x); } MSVC blindly uses the popcnt instruction whenever it sees this intrinsic. So this only works on Nehalem and newer Intel CPUs. And Barcelona and newer AMD CPUs. This is why llvm uses a bit math version of popcnt for MSVC in include/llvm/Support/MathExtras.h Comment at: include/bit:150 static_assert(sizeof(unsigned long long) == 8, ""); return __popcnt64(__x); } This doesn't exist in 32-bit MSVC. https://reviews.llvm.org/D50876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
mclow.lists added inline comments. Comment at: include/bit:96 #if defined(_LIBCPP_HAS_BITSCAN64) (defined(_M_AMD64) || defined(__x86_64__)) + if (_BitScanForward64(&__where, __x)) I'm not sure this code is ever used - since how can this compile? https://reviews.llvm.org/D50876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
mclow.lists updated this revision to Diff 161284.
mclow.lists added a comment.
Clean up the windows code a bit - though I don't think is used - since I don't
think it will compile.
https://reviews.llvm.org/D50876
Files:
include/__bit_reference
include/bit
Index: include/bit
===
--- include/bit
+++ include/bit
@@ -35,135 +35,123 @@
_LIBCPP_BEGIN_NAMESPACE_STD
+#ifndef _LIBCPP_COMPILER_MSVC
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned __x) { return __builtin_ctz(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned long __x) { return __builtin_ctzl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned long long __x) { return __builtin_ctzll(__x); }
+
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned __x) { return __builtin_clz(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned long __x) { return __builtin_clzl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned long long __x) { return __builtin_clzll(__x); }
+
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned __x) { return __builtin_popcount(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned long __x) { return __builtin_popcountl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned long long __x) { return __builtin_popcountll(__x); }
+
+#else // _LIBCPP_COMPILER_MSVC
+
// Precondition: __x != 0
inline _LIBCPP_INLINE_VISIBILITY
-unsigned __ctz(unsigned __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctz(__x));
-#else
+int __ctz(unsigned __x) {
static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
static_assert(sizeof(unsigned long) == 4, "");
- unsigned long where;
- // Search from LSB to MSB for first set bit.
- // Returns zero if no set bit is found.
- if (_BitScanForward(&where, __x))
-return where;
+ unsigned long __where;
+ if (_BitScanForward(&__where, __x))
+return static_cast(__where);
return 32;
-#endif
}
inline _LIBCPP_INLINE_VISIBILITY
-unsigned long __ctz(unsigned long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctzl(__x));
-#else
+int __ctz(unsigned long __x) {
static_assert(sizeof(unsigned long) == sizeof(unsigned), "");
return __ctz(static_cast(__x));
-#endif
}
inline _LIBCPP_INLINE_VISIBILITY
-unsigned long long __ctz(unsigned long long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctzll(__x));
-#else
-unsigned long where;
-// Search from LSB to MSB for first set bit.
-// Returns zero if no set bit is found.
+int __ctz(unsigned long long __x) {
+unsigned long __where;
#if defined(_LIBCPP_HAS_BITSCAN64)
(defined(_M_AMD64) || defined(__x86_64__))
- if (_BitScanForward64(&where, __x))
-return static_cast(where);
+ if (_BitScanForward64(&__where, __x))
+return static_cast(__where);
#else
// Win32 doesn't have _BitScanForward64 so emulate it with two 32 bit calls.
- // Scan the Low Word.
- if (_BitScanForward(&where, static_cast(__x)))
-return where;
- // Scan the High Word.
- if (_BitScanForward(&where, static_cast(__x >> 32)))
-return where + 32; // Create a bit offset from the LSB.
+ if (_BitScanForward(&__where, static_cast(__x)))
+return static_cast(__where);
+ if (_BitScanForward(&__where, static_cast(__x >> 32)))
+return static_cast(__where + 32);
#endif
return 64;
-#endif // _LIBCPP_COMPILER_MSVC
}
// Precondition: __x != 0
inline _LIBCPP_INLINE_VISIBILITY
-unsigned __clz(unsigned __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clz(__x));
-#else
+int __clz(unsigned __x) {
static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
static_assert(sizeof(unsigned long) == 4, "");
- unsigned long where;
- // Search from LSB to MSB for first set bit.
- // Returns zero if no set bit is found.
- if (_BitScanReverse(&where, __x))
-return 31 - where;
+ unsigned long __where;
+ if (_BitScanReverse(&__where, __x))
+return static_cast(31 - __where);
return 32; // Undefined Behavior.
-#endif
}
inline _LIBCPP_INLINE_VISIBILITY
-unsigned long __clz(unsigned long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clzl (__x));
-#else
+int __clz(unsigned long __x) {
static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
return __clz(static_cast(__x));
-#endif
}
inline _LIBCPP_INLINE_VISIBILITY
-unsigned long long __clz(unsigned long long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clzll(__x));
-#else
- unsigned long where;
-// BitScanReverse scans from MSB to LSB for first set bit.
-// Returns 0 if no set bit is found.
+int __clz(unsigned long long __x) {
+ unsigned long __where;
#if defined(_LIBCPP_HAS_BITSCAN64)
- if (_BitScanReverse64(&where, __x))
-return static_cast(63 - where);
+ if (_BitScanReverse64(&__wher
[PATCH] D50876: Clean up newly created header
mclow.lists added inline comments.
Comment at: include/bit:61
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned __x) { return __builtin_popcount (__x); }
+
ldionne wrote:
> Funny spacing between `__builtin_popcount` and `(__x)`
It's actually there to line up with the ones below.
(Though now that I look, I didn't do that for the ones above)
https://reviews.llvm.org/D50876
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
mclow.lists added inline comments. Comment at: include/bit:117 + unsigned long __where; // Search from LSB to MSB for first set bit. // Returns zero if no set bit is found. lebedev.ri wrote: > Like i commented in the original review, this should probably be > ``` > // Search from *M*SB to *L*SB for first set bit. > ``` I think my preference would be to just remove the comment. https://reviews.llvm.org/D50876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
lebedev.ri added inline comments. Comment at: include/bit:117 + unsigned long __where; // Search from LSB to MSB for first set bit. // Returns zero if no set bit is found. Like i commented in the original review, this should probably be ``` // Search from *M*SB to *L*SB for first set bit. ``` https://reviews.llvm.org/D50876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.
LGTM with the `__clz` you missed.
Comment at: include/bit:61
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned __x) { return __builtin_popcount (__x); }
+
Funny spacing between `__builtin_popcount` and `(__x)`
https://reviews.llvm.org/D50876
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
mclow.lists added inline comments.
Comment at: include/bit:113
inline _LIBCPP_INLINE_VISIBILITY
unsigned __clz(unsigned __x) {
static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
Missed this one. Should be `int`
https://reviews.llvm.org/D50876
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50876: Clean up newly created header
mclow.lists created this revision.
mclow.lists added reviewers: ldionne, EricWF.
Still NFC here.
1. Take the 9 functions that each had an `#ifndef _LIBCPP_COMPILER_MSVC` ..
`#else` .. `endif` block in them and make just two blocks, each with 9
functions. Much easier to read, but makes for a terrible diff.
2. Change the return type of `__ctz` and `__clz` from the same type as the
parameter to `int`. I would have preferred `unsigned`, but that's not what
P0553 gave us. I reviewed all the call sites for these functions, and for
`__ctz` it was always immediately static_cast-ed to another type, and `__clz`
it made no difference either.
3. Change the name `__pop_count` to `__popcount` to match the name that we're
going to add from P0553.
4. Rename the local variable in the windows code from `where` to `__where`.
Shame on someone ;-)
https://reviews.llvm.org/D50876
Files:
include/__bit_reference
include/bit
Index: include/bit
===
--- include/bit
+++ include/bit
@@ -35,135 +35,134 @@
_LIBCPP_BEGIN_NAMESPACE_STD
+#ifndef _LIBCPP_COMPILER_MSVC
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned __x) { return __builtin_ctz(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned long __x) { return __builtin_ctzl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned long long __x) { return __builtin_ctzll(__x); }
+
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned __x) { return __builtin_clz(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned long __x) { return __builtin_clzl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned long long __x) { return __builtin_clzll(__x); }
+
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned __x) { return __builtin_popcount (__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned long __x) { return __builtin_popcountl (__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned long long __x) { return __builtin_popcountll(__x); }
+
+#else // _LIBCPP_COMPILER_MSVC
+
// Precondition: __x != 0
inline _LIBCPP_INLINE_VISIBILITY
-unsigned __ctz(unsigned __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctz(__x));
-#else
+int __ctz(unsigned __x) {
static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
static_assert(sizeof(unsigned long) == 4, "");
- unsigned long where;
+ unsigned long __where;
// Search from LSB to MSB for first set bit.
// Returns zero if no set bit is found.
- if (_BitScanForward(&where, __x))
-return where;
+ if (_BitScanForward(&__where, __x))
+return static_cast(__where);
return 32;
-#endif
}
inline _LIBCPP_INLINE_VISIBILITY
-unsigned long __ctz(unsigned long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctzl(__x));
-#else
+int __ctz(unsigned long __x) {
static_assert(sizeof(unsigned long) == sizeof(unsigned), "");
return __ctz(static_cast(__x));
-#endif
}
inline _LIBCPP_INLINE_VISIBILITY
-unsigned long long __ctz(unsigned long long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctzll(__x));
-#else
-unsigned long where;
+int __ctz(unsigned long long __x) {
+unsigned long __where;
// Search from LSB to MSB for first set bit.
// Returns zero if no set bit is found.
#if defined(_LIBCPP_HAS_BITSCAN64)
(defined(_M_AMD64) || defined(__x86_64__))
- if (_BitScanForward64(&where, __x))
-return static_cast(where);
+ if (_BitScanForward64(&__where, __x))
+return static_cast(__where);
#else
// Win32 doesn't have _BitScanForward64 so emulate it with two 32 bit calls.
// Scan the Low Word.
- if (_BitScanForward(&where, static_cast(__x)))
-return where;
+ if (_BitScanForward(&__where, static_cast(__x)))
+return __where;
// Scan the High Word.
- if (_BitScanForward(&where, static_cast(__x >> 32)))
-return where + 32; // Create a bit offset from the LSB.
+ if (_BitScanForward(&__where, static_cast(__x >> 32)))
+return __where + 32; // Create a bit offset from the LSB.
#endif
return 64;
-#endif // _LIBCPP_COMPILER_MSVC
}
// Precondition: __x != 0
inline _LIBCPP_INLINE_VISIBILITY
unsigned __clz(unsigned __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clz(__x));
-#else
static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
static_assert(sizeof(unsigned long) == 4, "");
- unsigned long where;
+ unsigned long __where;
// Search from LSB to MSB for first set bit.
// Returns zero if no set bit is found.
- if (_BitScanReverse(&where, __x))
-return 31 - where;
+ if (_BitScanReverse(&__where, __x))
+return 31 - __where;
return 32; // Undefined Behavior.
-#endif
}
inline _LIBCPP_INLINE_VISIBILITY
-unsigned long __clz(unsigned long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clzl (__x));
-#else
+int
