Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove attempt to use tzcnt as bsf #2333

Merged
merged 6 commits into from
Nov 13, 2021

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Nov 10, 2021

Resolves #2330
Fixes VSO-1434895/AB#1434895.

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner November 10, 2021 17:27
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Nov 10, 2021
Copy link
Member

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How different is the codegen between the bsf version and the tzcnt version?

This is kinda pre-existing but we should do some benchmarks to determine if it's even worth doing the tzcnt version if we need to dynamically check for it.

@AlexGuteniev
Copy link
Contributor Author

but we should do some benchmarks to determine if it's even worth doing the tzcnt version if we need to dynamically check for it.

We'll track this as #2133

@AlexGuteniev
Copy link
Contributor Author

How different is the codegen between the bsf version and the tzcnt version?

Remember bsf requires a branch for zero input

@AlexGuteniev
Copy link
Contributor Author

AlexGuteniev commented Nov 11, 2021

@StephanTLavavej no, this wasn't really caused by #1540. Maybe some specific case was triggered by #1540, but the bug was always there since counr_zero implementation.

@StephanTLavavej
Copy link
Member

@AlexGuteniev Thanks, I was confused! I've restored your PR description.

@StephanTLavavej StephanTLavavej self-assigned this Nov 12, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 3ba0477 into microsoft:main Nov 13, 2021
@StephanTLavavej
Copy link
Member

Thanks for investigating and fixing this bug! 🐞 0️⃣ 1️⃣

@AlexGuteniev AlexGuteniev deleted the a_bit_wrong branch November 13, 2021 14:11
@AlexGuteniev
Copy link
Contributor Author

Fixed a bug affecting older CPUs where the STL generated the newer tzcnt instruction instead of the fallback bsf instruction. This affected 's countr_zero() and countr_one(), 's gcd() and lcm(), and vector's optimized implementations of find() and relational/spaceship operators. #2333

This is too aggressive. I mean probably gcd and lcm and vector are fine. For this bug to manifest we would need to immediately compare the result against zero. See @statementreply 's demo: https://godbolt.org/z/aPjnojjcx

I would have said "This affected 's countr_zero() and countr_one(), and potentially ..."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<bit>: countr_one(uint8) != 0 bad codegen on x86/x64
3 participants