GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||
GeezerVison is showing github.com — bpo-29882: Add _Py_popcount32() function by vstinner · Pull Request #20518 · python/cpython Conversation
|
|
@mdickinson: here you have. cc @pablogsal @1st1 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
On my x86-64 Fedora 32 with GCC 10.1.1, __builtin_popcount() becomes a call to __popcountdi2() function: It looks like the SWAR popcount. Using |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I wrote this PR to factorize duplicate function:
I also added |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This PR is a follow-up of PR #771 which added int.bit_count() Python function and popcount_digit() C function. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@mdickinson: I updated the PR. Would you mind to review it? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Will do, but not before the weekend. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments; mostly nitpicks, but there are some bits I don't understand (like the SIZEOF_INT == 8 branch).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Oh, 45af786 is good example of why I'm mentioning one used function on an include :-) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
* Rename pycore_byteswap.h to pycore_bitutils.h. * Move popcount_digit() to pycore_bitutils.h as _Py_popcount32(). * _Py_popcount32() uses GCC and clang builtin function if available. * Add unit tests to _Py_popcount32().
* Remove UINT32_C() * Fix #ifdef to decide if builtin functions are used of not: use SIZEOF_INT >= 4. Always use __builtin_popcountl() otherwise. * popcount_digit(): check PyLong_SHIFT rather than sizeof(digit). * Fix also two comments
|
@mdickinson: PR updated to address your latest review. PR rebased to fix a merge conflict with 45af786 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @mdickinson for the reviews! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
In pythonGH-2866, _Py_Bit_Length() was added to pymath.h for lack of a better location. pythonGH-20518 added a more appropriate header file for bit utilities. It also shows how to properly use intrinsics. This allows reconsidering bpo-29782. * Move the function to the new header. * Changed return type to match __builtin_clzl and reviewed usage. * Use intrinsics where available. * Pick a (mostly theoretical) fallback implementation suitable for inlining.
In GH-2866, _Py_Bit_Length() was added to pymath.h for lack of a better location. GH-20518 added a more appropriate header file for bit utilities. It also shows how to properly use intrinsics. This allows reconsidering bpo-29782. * Move the function to the new header. * Changed return type to match __builtin_clzl() and reviewed usage. * Use intrinsics where available. * Pick a fallback implementation suitable for inlining.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.