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

[Thomas Schatzl]

On Fri, 11 Dec 2020 12:34:11 GMT, Claes Redestad <redestad at openjdk.org> wrote:

>> src/hotspot/share/utilities/count_trailing_zeros.hpp line 98:
>> 
>>> 96:     if (index == 0) {
>>> 97:       _BitScanForward(&index, (uint32_t)(v >> 32));
>>> 98:       if (index > 0) {
>> 
>> Shouldn't this check be superfluous if the code previously asserted v != 0, and we know that the lower 32 bits are zero?
>> 
>> Also the MSDN documentation does not state that `index == 0` if no bit has been found, but the return value of the method is 0. I.e. shouldn't the implementation be something like:
>> 
>> if (_BitScanForward(&index, (uint32_t)v) == 0) { // no bit found? If so, try the upper dword. Otherwise index already contains the result
>>   _BitScanForward(&index, v >> 32); // no need for the uint32_t cast. >> is well defined on unsigned longs.
>>   // one could assert that the result of this invocation is non-zero, but it should be 1 if v != 0.
>>   index += 32;
>> }
>> return index;
>
> Yes, I misread the MSDN documentation and it appears index is undefined if there are no bits and we should rely on the return value.
> 
> Omitting the cast on `v >> 32` seems fine, but the comment is a bit off since `v` can be a signed type here.

You are right about `v` possibly being anything, I did not look correctly. But then the new code does indeed need some cast here because the result of `something signed >> 32` is compiler dependent (at least until C++20).

Since this code path is specific to Windows (i.e. Visual Studio), it might still be true that this is the case for all platforms (ie. even ARM64), but I'd prefer the explicit cast.

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

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