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

Update test for bsf private helper #2337

Merged
merged 3 commits into from
Dec 17, 2021

Conversation

AlexGuteniev
Copy link
Contributor

More coverage for #2330

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner November 13, 2021 13:30
@CaseyCarter CaseyCarter added the test Related to test code label Nov 13, 2021
@barcharcraz barcharcraz removed their assignment Dec 10, 2021
@StephanTLavavej StephanTLavavej self-assigned this Dec 16, 2021
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Dec 16, 2021
@StephanTLavavej
Copy link
Member

I've pushed changes, now affecting product code, to fix /clr:pure test failures internally. The issue is that because the test was being run for all Standard modes (including /clr:pure), _Countr_zero_bsf was being directly tested, even though it's used for C++20 only (after the logic was fixed for C++17 constexpr gcd()). Changelog:

  • <limits>
    • Centralize _HAS_TZCNT_BSF_INTRINSICS. Same architecture guard as before, but additionally block /clr:pure, CUDA, and ICC. (This makes it equivalent to _HAS_POPCNT_INTRINSICS but I didn't want to unify them here.)
    • Guard the logic with _HAS_TZCNT_BSF_INTRINSICS. __isa_available needs to be available for popcnt below, so don't guard it any more restrictively.
    • Additionally guard the tzcnt/bsf logic with _HAS_CXX20, since that's the only mode it's used in (no reason to define it and never use it).
    • Extract the _TZCNT_U32/_TZCNT_U64 macro definitions from the extern "C" block (they don't need to be within it) so they can be guarded by _HAS_CXX20.
    • Immediately after _Countr_zero_tzcnt, undef those macros, instead of doing it later.
    • In _Countr_zero, change the guard to _HAS_CXX20 && _HAS_TZCNT_BSF_INTRINSICS; we need the former for is_constant_evaluated and the latter for _Checked_x86_x64_countr_zero.
    • #undef _HAS_TZCNT_BSF_INTRINSICS at the end of the file.
  • env.lst
    • Go back to the usual_20_matrix.lst.
  • test.cpp
    • Drop _HAS_CXX20 guards, as we're always in C++20 mode.

@StephanTLavavej StephanTLavavej merged commit f3ab9de into microsoft:main Dec 17, 2021
@StephanTLavavej
Copy link
Member

Thanks for improving this test coverage! 🧪 ✅ 🚀

@AlexGuteniev AlexGuteniev deleted the coverage_for_bsf_tzcnt branch December 17, 2021 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved test Related to test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants