Conversation
|
Ohh fingers crossed, but from a glance seems relevant. |
| // BitScanReverse gives the bit position (0 for the LSB, then 1, etc.) of the | ||
| // first bit that is 1, when looking from the MSB. To count leading zeros, we | ||
| // need to adjust that. | ||
| return 31 - int(count); |
|
Just tried out using the binary from CI artifacts, and it does fix the issue! |
|
It's already noticed by testing, so no need to add a new test I think. The problem is we can't enable windows tests on CI as several other failures exist. One option might be to try to enable at least some tests as they pass. |
|
Oh, I see, I thought it was something not covered by tests. Are those generic failures? How hard would it be to fix them? Either way, yeah, at least enabling some tests could be better than nothing. (not in scope of this PR though) |
|
There is at least #2781, an issue with Not sure how hard they are to fix. Complicating this is that I don't have a windows machine and can't seem to access the normal windows VM I've been using. I'll look into that, but meanwhile I debugged this particular issue using CI here. Would be good if someone with a windows machine could find some time for this. |
I think a VM I mentioned earlier on the issue should be enough for such sort of debugging and it includes most of necessary tooling:
But, of course, it's up to you whether you want to try that or leave to someone else with "proper" Windows machine :) |
|
Ah, I don't think I can run those windows VMs locally either... anyhow, I'm probably the worst person for windows-specific issues as I would need to figure out basics like getting cmake and such, it's been a very long time since I did windows development. |
We just had the logic there wrong - MSVC's intrinsic returns the bit
index, not the number of leading zeros. That's identical when scanning
forward but not in reverse...
This fixes one set of windows (or rather MSVC) specific errors on CI,
which I verified on CI. (But other issues remain after this, so we can't
turn on windows CI yet.)
This hopefully will fix #2942 , cc @RReverser