bpo-29882: Fix portability bug introduced in GH-30774

[mdickinson]

This PR fixes a portability bug in _Py_popcount32 that was introduced in GH-30774, and adds a comment explaining why the final line of the function is delicate.

Prior discussions:

• bpo-29882: Add an efficient popcount method for integers #771 (comment)
• bpo-29882: Add _Py_popcount32() function #20518 (comment)
• bpo-29882: _Py_popcount32() doesn't need 64x64 multiply #30774 (comment)

https://bugs.python.org/issue29882

[vstinner]

Can you please add a test for 2**28 + 1 in _testinternalcapi.test_popcount()?

ref: #30774 (comment)

[mdickinson]

Can you please add a test for 2**28 + 1 in _testinternalcapi.test_popcount()?

Sure, will do. Though note that there's really nothing at all special about that value: any input value larger than 255 will give the wrong result under the code currently in main, on a machine with 64-bit int.

[vstinner]

Adding (uint32_t) cast and the added test LGTM. I didn't review the long comment ;-)

[vstinner]

Why removing this constant? I added it to not have to use the macro to get an uint32_t literal number. UINT32_C() if I recall correctly.

[mdickinson]

@vstinner This is already explained in the long comment that you didn't review. :-)

The problem here isn't the constant; it's the type declaration.

For the multiplication in the last line of the function to be portable, we need at least one of the unsigned operands in that multiplication to not be promoted to int. An inline constant 0x01010101U satisfies that criterion: by C99 §6.4.4.1, together with C's guarantees about the minimum precision of long, it has type either unsigned int or unsigned long. A uint32_t constant with the same value does not satisfy that criterion, for reasons already explained.

And there isn't a type declaration that works here. I just explained why uint32_t won't work. If we declare SUM as unsigned int instead of uint32_t, we have portability issues on machines where int isn't large enough to represent the value 0x01010101. If we declare it as unsigned long, we're back to doing a 64-bit-by-64-bit multiply on almost all current Linux and macOS boxes.

If you really want to keep the constant, another option is to leave this line exactly as-is and change the multiplication in the last line to x * (SUM + 0U); that + 0U effectively forces the second multiplicand to have type with rank greater than or equal to that of int, making it immune to further integer promotion.

But as Tim observed in the #30774 discussion, none of this prevents us from potentially doing a 512-bit-by-512-bit multiply on a box that has 512-bit integers. But short of relying on compiler-specific intrinsics, that's inescapable anyway: standard C simply isn't capable of doing arithmetic on anything smaller than an int.

[tim-one]

Looks fine to me!

[bedevere-bot]

@mdickinson: Please replace # with GH- in the commit message next time. Thanks!