bpo-29782: Consolidate _Py_Bit_Length() by niklasf · Pull Request #20739 · python/cpython

niklasf · 2020-06-08T18:08:20Z

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 (mostly theoretical) fallback implementation suitable for
inlining.

https://bugs.python.org/issue29782

vstinner

Maybe add test_bit_length() to _testinternalcapi.c: see test_popcount().

vstinner · 2020-06-08T18:18:16Z

Previous rejected attempt: PR #594.

vstinner · 2020-06-08T18:19:58Z

cc @mdickinson

vstinner

I disliked PR #594 because it added #include <intrin.h> to a new *public header file: Include/pyintrinsics.h. This PR is different, all changes are made in the internal C API. We have way more freedom in the internal C API.

vstinner · 2020-06-08T18:25:05Z

The Windows build looks broken:

Run .\python.bat -m test.pythoninfo
Running Release|x64 interpreter...
##[error]Process completed with exit code 1.

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.

Co-authored-by: Victor Stinner <vstinner@python.org>

niklasf · 2020-06-08T21:03:35Z

Thanks for reviewing.

Also added unit tests as suggested (in addition to the existing test coverage). I briefly disabled the branch with clz (8e3ce66) to get a CI run with the fallback implementation. The current fallback is identical to the original (except for inlining).

vstinner

Thanks for the many updates! The code now looks better! New review.

Co-authored-by: Victor Stinner <vstinner@python.org>

vstinner · 2020-06-08T22:25:54Z

Using gcc 10 -O3 -flto, I get the following machine code in test_bit_length():

# input = rax
# if (x != 0)
test rax,rax
je 0x7fffeaa99978 <test_bit_length+504>
# return (int)sizeof(unsigned long) * 8 - __builtin_clzl(x);
# return 0x40 - bsr(rax) ^ 0x3f
bsr rax,rax
mov ecx,0x40
xor rax,0x3f
mov r9d,ecx
sub r9d,eax
# result = r9d

x86 BSR (Bit Scan Reverse) instruction is used as expected.

vstinner

LGTM!

@mdickinson: Do you want to review this PR as well?

mdickinson · 2020-06-09T17:27:54Z

@mdickinson: Do you want to review this PR as well?

I'd very much like to take a look, but am short on available time. Can you give me until the weekend? If I've failed to review by the start of next week, go ahead and merge.

vstinner · 2020-06-09T17:56:38Z

@mdickinson: this PR can wait one week :-)

mdickinson · 2020-06-14T09:00:07Z

+static int
+bit_length_digit(digit x)
+{
+ Py_BUILD_ASSERT(PyLong_SHIFT <= sizeof(unsigned long) * 8);


It would be nice if C provided some standard way to get the width (in the sense of C99 §6.2.6.2p6) of an integer type, so that we didn't have to use sizeof(unsigned long) * 8, which could fail in the presence of padding bits. But as far as I know it doesn't. Related: https://stackoverflow.com/q/3957252/270986

mdickinson

LGTM!

vstinner · 2020-06-15T12:44:34Z

Thanks @niklasf, I merged your PR.

I removed "(mostly theoretical)" from your commit message:

* Pick a (mostly theoretical) fallback implementation suitable for
inlining.

We support other C compilers, like XLC.

the-knights-who-say-ni added the CLA signed label Jun 8, 2020

bedevere-bot added the awaiting review label Jun 8, 2020

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadInclude/internal/pycore_bitutils.h Outdated
Comment threadInclude/internal/pycore_bitutils.h Outdated
Comment threadInclude/internal/pycore_bitutils.h Outdated
Comment threadModules/mathmodule.c Outdated

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadInclude/internal/pycore_bitutils.h Outdated

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadModules/mathmodule.c Outdated

mdickinson self-requested a review June 8, 2020 18:26

niklasf added 4 commits June 8, 2020 21:18

Specify required gcc version for __builtin_clzl
65abb83

Fix _BitScanReverse usage
ce45ff2

Document reason for include
6464ca6

niklasf force-pushed the bpo-29782-msb branch from 409d58c to e7992adCompare June 8, 2020 19:43

Ensure future compatibility
0bd4265

niklasf force-pushed the bpo-29782-msb branch from e7992ad to 0bd4265Compare June 8, 2020 19:50

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadInclude/internal/pycore_bitutils.h Outdated
Comment threadModules/mathmodule.c Outdated

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadInclude/internal/pycore_bitutils.h Outdated

Do not try to support GCC < 3.4
fbaca66

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadInclude/internal/pycore_bitutils.h Outdated

niklasfand others added 2 commits June 8, 2020 22:03

Coding style
436f68b

Fix comment indent
ac664bc
Co-authored-by: Victor Stinner <vstinner@python.org>

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadInclude/internal/pycore_bitutils.h Outdated
Comment threadInclude/internal/pycore_bitutils.h Outdated

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadInclude/internal/pycore_bitutils.h

niklasf added 6 commits June 8, 2020 22:21

Do not factor out x != 0
3279d17

Fix and move _Py_bit_length_digit()
525da3d

Add unit test for _Py_bit_length()
e487622

Remove useless cast in test
a338761

Use original implementation as fallback
5a146f7

Re-enable intrinsics
8e3ce66

vstinner reviewed Jun 8, 2020
View reviewed changes

Comment threadInclude/internal/pycore_bitutils.h
Comment threadModules/_testinternalcapi.c Outdated
Comment threadModules/_testinternalcapi.c Outdated
Comment threadObjects/longobject.c Outdated
Comment threadInclude/internal/pycore_bitutils.h Outdated

niklasfand others added 7 commits June 8, 2020 23:12

Explicitly cast sizeof
86241f6
Co-authored-by: Victor Stinner <vstinner@python.org>

Better tests
c7ccbe2
Co-authored-by: Victor Stinner <vstinner@python.org>

Explain Py_BUILD_ASSERT(sizeof(unsigned long) <= 4)
dfceaf2

Use volatile so test actually runs intrinsic
fbf01ae

Remove trailing spaces
991622b

_Py_bit_length_digit -> bit_length_digit
462d995

Fixup test expectations
98fc7a8

vstinner added the skip news label Jun 8, 2020

vstinner approved these changes Jun 8, 2020
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jun 8, 2020

mdickinson reviewed Jun 14, 2020
View reviewed changes

mdickinson approved these changes Jun 14, 2020
View reviewed changes

vstinner merged commit 794e7d1 into python:masterJun 15, 2020

bedevere-bot removed the awaiting merge label Jun 15, 2020

Uh oh!

Uh oh!

Conversation

niklasf commented Jun 8, 2020• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jun 8, 2020

Uh oh!

Uh oh!

vstinner commented Jun 8, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Jun 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasf commented Jun 8, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jun 8, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

mdickinson commented Jun 9, 2020

Uh oh!

vstinner commented Jun 9, 2020

Uh oh!

mdickinsonJun 14, 2020

Choose a reason for hiding this comment

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jun 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

niklasf commented Jun 8, 2020•
edited by bedevere-bot
Loading