`bit::lzcnt` does not work as intended on MSVC when running on a CPU without BMI1 instruction set
MSVC builds aren't really tested much, I would assume that tzcnt is broken in the same way then.
Would gladly take a PR, I neither have an MSVC build system right now other than abusing our CI, nor access to problematic hardware since even my old Phenom supports lzcnt.
FWIW, __builtin_clz is based on bsr.
Have you checked whether you have enabled "Enable Enhanced Instruction Set" in project properties? Based on my own experience, once you set this to AVX or AVX2, then MSVC will always assume BMI1 is available and will crash on old CPU. By default this should be set to SSE2 for all x64 compatible CPU, but in some cases this property may be modified. You should find this property under "Configuration Properties > C/C++ > Code Generation".
If you are directly using command line to compile with MSVC, it should appear as "/arch:xxx", e.g. "/arch:AVX2" will always assume AVX2 available.
@qinlili23333 As far as I understand, the problem here is that MSVC intrinsics always emit the instruction even if the target architecture doesn't include support for it.
@qinlili23333 As far as I understand, the problem here is that MSVC intrinsics always emit the instruction even if the target architecture doesn't include support for it.
Then it seems to be a MSVC bug. But as far as I know, I haven't face this issue on my own cpp projects. I'll do some test to see how actually MSVC works. I have just revived one old laptop with 3337U which lacks BMI1 and AVX2 several weeks ago.
Some related info from MS STL repo:
https://github.com/microsoft/STL/issues/1103
https://github.com/microsoft/STL/commit/782cd650ec5ee95feb8128b1223f4f60987f84eb
Seems they fixed it in 2020.
Use some code modified from https://github.com/microsoft/STL/issues/1103#issuecomment-665820330
Checkout my code here: https://github.com/qinlili23333/LZCNT-MSVC-Test
Artifact compiled with Microsoft Visual Studio Community 2022 (64-bit) - Preview Version 17.14.0 Preview 2.0, with Microsoft (R) C/C++ Optimizing Compiler Version 19.44.34918.1
LZCNT.zip
On 1135G7 (up to AVX2), all variants behave the same as native LZCNT.
On 3337U (up to AVX1 only), all variants behave the same as BSR fallback.
I don't see anything that would crash on a CPU without BMI1, cannot reproduce.
@PiMoNFeeD Can you test these artifacts on the machine you got crashing?
The crash doesn't happen because of the instruction itself (it is encoded as rep bsr, so on CPUs that don't support lzcnt it is literally a bsr instruction), but because the result is incorrect and things blow up inside the allocator logic later. bsr has different semantics than lzcnt even for non-zero inputs.
Have you checked whether you have enabled "Enable Enhanced Instruction Set" in project properties? Based on my own experience, once you set this to AVX or AVX2, then MSVC will always assume BMI1 is available and will crash on old CPU. By default this should be set to SSE2 for all x64 compatible CPU, but in some cases this property may be modified. You should find this property under "Configuration Properties > C/C++ > Code Generation". If you are directly using command line to compile with MSVC, it should appear as "/arch:xxx", e.g. "/arch:AVX2" will always assume AVX2 available.
Yes, it builds with SSE2 and not AVX2.
I don't see anything that would crash on a CPU without BMI1, cannot reproduce.
@PiMoNFeeD Can you test these artifacts on the machine you got crashing?
The results are:
Release\LZCNT.exe
BSR: 27 LZCNT: 27
The rest of the executables (all the AVX ones) didn't launch, which is to be expected from a CPU that does not support AVX.
@qinlili23333 did you build Release with SSE2 enabled, or no enhanced instructions at all?
@qinlili23333 did you build Release with SSE2 enabled, or no enhanced instructions at all?
SSE2 does not need to be enabled, it's always enabled according to MSVC doc, as all x64 processor should support it. https://learn.microsoft.com/en-us/cpp/build/reference/arch-x64?view=msvc-170
The crash doesn't happen because of the instruction itself (it is encoded as
rep bsr, so on CPUs that don't support lzcnt it is literally absrinstruction), but because the result is incorrect and things blow up inside the allocator logic later. bsr has different semantics than lzcnt even for non-zero inputs.
Did some further research. Sorry I'm not very familiar with these modern asm codes. Seems the design of asm code "rep bsr" is actually a bad design that it just allows a "fallback" to wrong value. And MSVC didn't detect CPUID then just compiled as the CPU should support it, which resulted wrong value.
Anyway I'll try to figure out some fix with CPUID detection.
The proper fix here would be to just use bsr intrinsics that MSVC offer, with (cached) cpuid detection we'd spend more time branching than unconditionally executing one single bsr -> cmov -> sub sequence even if bsr is quite a bit slower than lzcnt.
Technically it may also be possible to abuse the zero-flag behaviour of lzcnt 0 to figure out whether we're running bsr or lzcnt similar to how we do it for tzcnt, but again that's likely slower than just doing one bsr + some fix-up.
Did some benchmark. I agree that just bsr is the fastest way.
Benchmark cod: https://github.com/qinlili23333/LZCNT-MSVC-Test/blob/benchmark/LZCNT/LZCNT.cpp
I'll make a PR later.
@PiMoNFeeD Can you test this branch on your machine? https://github.com/qinlili23333/dxvk/tree/bsr_fix
I just found that my old laptop GPU is Fermi based which is not compatible with dxvk so I cannot finish the test.
@PiMoNFeeD Can you test this branch on your machine? https://github.com/qinlili23333/dxvk/tree/bsr_fix I just found that my old laptop GPU is Fermi based which is not compatible with dxvk so I cannot finish the test.
I think you updated the wrong function, because _BitScanReverse actually accepts a DWORD as a second argument. You need to use _BitScanReverse64 for uint64_t.
UPD: actually it only exists on 64 bit builds... not sure how to deal with that then?
I think you updated the wrong function, because
_BitScanReverseactually accepts a DWORD as a second argument. You need to use_BitScanReverse64foruint64_t.
UPD: actually it only exists on 64 bit builds... not sure how to deal with that then?
Yeah I have to use _BitScanReverse just to keep 32-bit compiled. I'm also a bit confused about how to deal it on both 32 and 64 bits platforms.
I think I have found some solution, wait me some time.
@PiMoNFeeD Pushed another commit and this should fix it. https://github.com/doitsujin/dxvk/pull/4811/commits/8e5dd0cf1496ac2a200a2f43d664f6d8e6643299
@PiMoNFeeD Pushed another commit and this should fix it. 8e5dd0c
Works on my end and for affected machine. Thank you!
I wonder now though, tzcnt should also be affected... but from what I understand, tzcnt is the same as bsf for non-zero inputs, which I assume is the case most of the time for DXVK? Do you think it's worth fixing that too?
I wonder now though,
tzcntshould also be affected... but from what I understand,tzcntis the same asbsffor non-zero inputs, which I assume is the case most of the time for DXVK? Do you think it's worth fixing that too?
Thanks for the information. I'll do some test to check it.
EDIT: I have checked TZCNT, it appears same on 3337U and 11800H since TZCNT is same with BSF for non-zero. So it doesn't need further fix.
we need proper tzcnt semantics for zero inputs as well, that's the whole point of these functions. Returning random garbage for zero inputs will break things too.
we need proper tzcnt semantics for zero inputs as well, that's the whole point of these functions.
I'll figure out it and combile it in the same PR.
Upstream links
- doitsujin/dxvk/pull/4811x1 2025-03
I have a DirectX game I build with MSVC, and I recently added support for Vulkan similar to how Valve did it in Source Engine by directly compiling DXVK sources into the binaries and not linking DirectX SDK libs. The problem is that one of my players reported a crash on startup somewhere in memory allocator, which I quickly traced to
bit::lzcnt.While trying to allocate 4096 bytes of memory, a
DxvkPoolAllocator::computeListIndexcall is made, which calls intobit::lzcntand then_lzcnt_u32. On any CPU made in the last ~15 years, this call with 4096 as argument returns 3. For this user, it returns 4294967290 instead. Under MSVC, it simply calls the instruction directly, without checking if it's supported by the CPU or not. The player's CPU is i5 750, and it does not support that instruction. Release DLLs from this repository do not crash for them, since they are not built with MSVC and I assume either__builtin_clzis backwards-compatible with the old CPUs, or your compiler falls back to your own implementation of this instruction.I didn't look into it much, but I think re-implementing the instruction using
_BitScanReverseshould fix the issue.