RFR: 8257985: count_trailing_zeros doesn't handle 64-bit values on 32-bit JVM [v7]

[Kim Barrett]

On Fri, 11 Dec 2020 14:07:08 GMT, Claes Redestad <redestad at openjdk.org> wrote:

>> Re-implement count_trailing_zeros as a template function similar to count_leading_zeros, adding support for 8 and 16 bit types as well as for 64-bit types on 32-bit builds, which is currently not supported. This prohibits implementing the log2i_exact proposed by #1663 using count_trailing_zeros, as suggested during review.
>> 
>> Compared to count_leading_zeros the implementation can be slightly simpler since subword specializations can simply use the internal 32-bit variant.
>> 
>> Windows doesn't define _BitScanForward64 on 32-bit targets, so the 32-bit variant for handling 64-bit values becomes more convoluted.
>
> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Explicit static_cast

Changes requested by kbarrett (Reviewer).

src/hotspot/share/utilities/count_trailing_zeros.hpp line 81:

> 79:   if (_BitScanForward(&index, (uint32_t)x) == 0) {
> 80:     // no bit found? If so, try the upper dword. Otherwise index already contains the result
> 81:     _BitScanForward(&index, x >> 32);

If the first BSF needs a cast then this one probably does too.  The shift result is still uint64_t.

src/hotspot/share/utilities/count_trailing_zeros.hpp line 79:

> 77:   _BitScanForward64(&index, x);
> 78: #else
> 79:   if (_BitScanForward(&index, (uint32_t)x) == 0) {

Is the cast needed?  That is, does the implicit conversion from uint64_t to (32bit) unsigned long not work?  If it's needed, please use static_cast.

src/hotspot/share/utilities/count_trailing_zeros.hpp line 82:

> 80:     // no bit found? If so, try the upper dword. Otherwise index already contains the result
> 81:     _BitScanForward(&index, x >> 32);
> 82:     assert(index > 0, "invariant since x != 0");

I'm not sure this assert really does anything useful.  If both of the BSFs failed because the value is zero (which shouldn't happen because we checked for that), then the value of index is undefined.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1747