bpo-29782: Consolidate _Py_Bit_Length() #20739

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

Include/internal/pycore_bitutils.h

Modules/mathmodule.c

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

Previous rejected attempt: PR #594.

Include/internal/pycore_bitutils.h

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.

Modules/mathmodule.c

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.

Include/internal/pycore_bitutils.h

Modules/mathmodule.c

Include/internal/pycore_bitutils.h

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

Include/internal/pycore_bitutils.h

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.

Include/internal/pycore_bitutils.h

Modules/_testinternalcapi.c

Objects/longobject.c

Include/internal/pycore_bitutils.h

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

Objects/longobject.c

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

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 fallback implementation suitable for inlining.

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

Include/internal/pycore_bitutils.h Show resolved Hide resolved

Include/internal/pycore_bitutils.h Outdated Show resolved Hide resolved

Include/internal/pycore_bitutils.h Show resolved Hide resolved

Modules/mathmodule.c Outdated Show resolved Hide resolved

vstinner reviewed Jun 8, 2020

View reviewed changes

Include/internal/pycore_bitutils.h Outdated Show resolved Hide resolved

vstinner reviewed Jun 8, 2020

View reviewed changes

Modules/mathmodule.c Show resolved Hide resolved

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 e7992ad Compare June 8, 2020 19:43

Ensure future compatibility

0bd4265

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

vstinner reviewed Jun 8, 2020

View reviewed changes

Include/internal/pycore_bitutils.h Outdated Show resolved Hide resolved

Modules/mathmodule.c Outdated Show resolved Hide resolved

vstinner reviewed Jun 8, 2020

View reviewed changes

Include/internal/pycore_bitutils.h Outdated Show resolved Hide resolved

Do not try to support GCC < 3.4

fbaca66

vstinner reviewed Jun 8, 2020

View reviewed changes

Include/internal/pycore_bitutils.h Outdated Show resolved Hide resolved

niklasf and 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

Include/internal/pycore_bitutils.h Outdated Show resolved Hide resolved

Include/internal/pycore_bitutils.h Outdated Show resolved Hide resolved

vstinner reviewed Jun 8, 2020

View reviewed changes

Include/internal/pycore_bitutils.h Show resolved Hide resolved

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

niklasf and 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:master Jun 15, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-29782: Consolidate _Py_Bit_Length() #20739

bpo-29782: Consolidate _Py_Bit_Length() #20739

niklasf commented Jun 8, 2020 •

edited by bedevere-bot

Loading

vstinner left a comment

vstinner commented Jun 8, 2020

vstinner commented Jun 8, 2020

vstinner left a comment

vstinner commented Jun 8, 2020

niklasf commented Jun 8, 2020

vstinner left a comment

vstinner commented Jun 8, 2020

vstinner left a comment

mdickinson commented Jun 9, 2020

vstinner commented Jun 9, 2020

mdickinson Jun 14, 2020

mdickinson left a comment

vstinner commented Jun 15, 2020

bpo-29782: Consolidate _Py_Bit_Length() #20739

bpo-29782: Consolidate _Py_Bit_Length() #20739

Conversation

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

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Jun 8, 2020

vstinner commented Jun 8, 2020

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Jun 8, 2020

niklasf commented Jun 8, 2020

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Jun 8, 2020

vstinner left a comment

Choose a reason for hiding this comment

mdickinson commented Jun 9, 2020

vstinner commented Jun 9, 2020

mdickinson Jun 14, 2020

Choose a reason for hiding this comment

mdickinson left a comment

Choose a reason for hiding this comment

vstinner commented Jun 15, 2020

niklasf commented Jun 8, 2020 •

edited by bedevere-bot

Loading