<bit>: Concern about lzcnt used as bsr correctness

[AlexGuteniev]

@statementreply asks:

I’m concerned about the correctness of our x86/x64 implementation of countl_zero, on CPUs without LZCNT support.

// We use lzcnt (actually bsr if lzcnt is not supported)

bsr(x) == integer_width - 1 - lzcnt(x) when x != 0, so the fallback won’t work.
I currently don’t have access to a computer with pre-AVX2 CPU. Could someone help testing this?

[StephanTLavavej]

I believe this is indeed a bug; we’ll need pre-Haswell hardware to validate a fix.

[fsb4000]

I have AMD FX8300, which doesn't support AVX2, only first AVX. If you write test I can compile and run it...

[AlexGuteniev]

The program that demonstrates the problem is as follows:

#include <intrin.h>
#include <iostream>

int main()
{
    volatile unsigned i = 0x0FFF'FFFC;

    unsigned long bsf;
    _BitScanForward(&bsf, i);
    unsigned tzcnt = __builtin_ctz(i);

    unsigned long bsr;
    _BitScanReverse(&bsr, i);
    unsigned lzcnt = __lzcnt(i);
    
    std::cout << "BSF:" << bsf << " TZCNT:" << tzcnt << " BSR:" << bsr << " LZCNT:" << lzcnt << "\n";
}

Expected output with LZCNT/TZCNT support:

BSF:2 TZCNT:2 BSR:27 LZCNT:4

Expected output without support:

BSF:2 TZCNT:2 BSR:27 LZCNT:27

But it would be more useful to validate fix, not bug.
For this, need first the PR itself, and the second run it on old CPU

[AlexGuteniev]

I see as a good fix as applying the correction after lzcnt call.
But for this fix to work properly need to expose ABM CPUID bit, and don't rely on __ISA_AVAILABLE_AVX2, since unneccessary correction is also a bug.

As a not so good fix, we can just use _BitScanReverse and correct it always.

[CaseyCarter]

I think the best fix will be to give up on always using the same instruction - it's too awkward to both check for 0 before and possibly adjust after. I think we should simply switch on availability and use either lzcnt or bsr.

[AlexGuteniev]

I think the best fix will be to give up on always using the same instruction - it's too awkward to both check for 0 before and possibly adjust after. I think we should simply switch on availability and use either lzcnt or bsr.

It can be done as adjust after, and handle 0 case as separate branch of adjustment.

[AlexGuteniev]

Like this:

template <class _Ty>
_NODISCARD int _Unchecked_x86_x64_countl_zero(const _Ty _Val) noexcept {
    constexpr int _Digits  = numeric_limits<_Ty>::digits;

    // We use lzcnt (actually bsr if lzcnt is not supported) now that we know
    // we're not zero. We can do this because lzcnt and bsr share the same instruction
    // encoding.
    if constexpr (_Digits <= 16) {
        return static_cast<int>(__lzcnt16(_Val) - (16 - _Digits));
    } else if constexpr (_Digits == 32) {
        return static_cast<int>(__lzcnt(_Val));
    } else {
#ifdef _M_IX86
        static_assert(_Digits <= 32, "Should have handled this in _Checked_x86_x64_countl_zero");
#else // ^^^ _M_IX86 / !_M_IX86 vvv
        return static_cast<int>(__lzcnt64(_Val));
#endif // _M_IX86
    }
    // note: we don't need to call a fallback here because
    // all supported x86 processors at least have bsr/bsf
}

template <class _Ty>
_NODISCARD int _Checked_x86_x64_countl_zero(const _Ty _Val) noexcept {
    if constexpr (sizeof(_Ty) > sizeof(void*)) {
        const unsigned int _High = _Val >> 32;
        const auto _Low          = static_cast<unsigned int>(_Val);
        if (_High == 0) {
            return 32 + _Checked_x86_x64_countl_zero(_Low);
        } else {
            return _Checked_x86_x64_countl_zero(_High);
        }
    } else {
        int _Result            = _Unchecked_x86_x64_countl_zero(_Val);
#ifndef __AVX2__
        const bool _Have_lzcnt = __have_abm;
        // lzcnt (when it doesn't fall back to bsr) is defined correctly for zero
        // bsr has undefined output for zero
        if (!_Have_lzcnt) {
            if (_Val == 0) {
                _Result = 0;
            } else {
                // bsr counts from least significant bit, lzcnt counts from most significant bit, apply correction
                constexpr int _Digits = numeric_limits<_Ty>::digits;
                _Result               = _Digits - 1 - _Result;
            }
        }
#endif // __AVX2__
        return _Result;
    }
}

[AlexGuteniev]

Actually, I think I can finalize this to a PR, I know how to do it without magic static or magic extern...

[CaseyCarter]

Good point. I think we also need to revert the "fixup" for 8-bit arguments in return static_cast<int>(__lzcnt16(_Val) - (16 - _Digits)); before performing the post-adjustment.

[fsb4000]

Expected output with LZCNT/TZCNT support:

BSF:2 TZCNT:2 BSR:27 LZCNT:4

Expected output without support:

BSF:2 TZCNT:2 BSR:27 LZCNT:27

But it would be more useful to validate fix, not bug.
For this, need first the PR itself, and the second run it on old CPU

Sure, will do. For now, I just do some screenshots to confirm the bug: https://imgur.com/a/zsiBqym

[AlexGuteniev]

I just do some screenshots to confirm the bug: https://imgur.com/a/zsiBqym

Then it is not old enough, it supports ABM.

[fsb4000]

Then it is not old enough, it supports ABM.

Oh, ok.

Sorry, I can't help...

I have another PC with AMD Phenom II 925, but it is also support ABM.

https://en.wikipedia.org/wiki/List_of_AMD_Phenom_microprocessors#%22Deneb%22_(C2/C3,_45_nm,_Quad-core)

All models support: MMX, SSE, SSE2, SSE3, SSE4a, ABM, Enhanced 3DNow!, NX bit, AMD64, Cool'n'Quiet, AMD-V