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

Concentrated header for internal bit utilities #3721

Merged
merged 10 commits into from
Jun 15, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented May 20, 2023

Also reduces inclusion dependency on <limits>.

Currently __isa_available and _Stl_isa_available_meow are made unconditionally declared, but it might be better to avoid declaring them under some circumstances.

Fixes #3692. Fixes #832.

MSVC-internal changes are needed for the new header <__msvc_bit_utils.hpp>.

Also reduces inclusion dependency on `<limits>`.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner May 20, 2023 09:49
@CaseyCarter CaseyCarter added enhancement Something can be improved throughput Must compile faster and removed enhancement Something can be improved labels May 24, 2023
#endif // _HAS_NEON_INTRINSICS

template <class _Ty>
_INLINE_VAR constexpr bool _Is_standard_unsigned_integer =
Copy link
Member

Choose a reason for hiding this comment

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

No change requested: Nice catch!! We mistakenly lacked _INLINE_VAR here:

STL/stl/inc/limits

Lines 1204 to 1205 in c8d1efb

template <class _Ty>
constexpr bool _Is_standard_unsigned_integer =

@StephanTLavavej StephanTLavavej removed their assignment Jun 12, 2023
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej self-assigned this Jun 14, 2023
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej
Copy link
Member

I pushed a conflict-free merge with main, then I had to push a commit to guard the lzcnt etc. intrinsic usage because /clr:pure (and CUDA/Intel) doesn't understand them. We didn't need it before when this was in <bit> which is C++20, but now this code is being promoted to C++14 mode.

Comment on lines +67 to +68
#if !defined(_M_CEE_PURE) && !defined(__CUDACC__) && !defined(__INTEL_COMPILER)
#define _HAS_COUNTL_ZERO_INTRINSICS 1
Copy link
Member

Choose a reason for hiding this comment

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

Note to other reviewers: I kept this structure for consistency with our other intrinsics guards, even though this one could arguably be De Morganed.

@StephanTLavavej StephanTLavavej merged commit 47679bb into microsoft:main Jun 15, 2023
@StephanTLavavej
Copy link
Member

Thanks for this major header restructuring! 🛠️ 🎉 😸

@StephanTLavavej
Copy link
Member

I observe that <bitset> and <numeric> are still indirectly dragging in <limits> which we probably should have checked in the first place, but this is still a reasonable restructuring.

@frederick-vs-ja frederick-vs-ja deleted the bit-utils branch June 15, 2023 09:00
@frederick-vs-ja
Copy link
Contributor Author

I observe that <bitset> and <numeric> are still indirectly dragging in <limits> which we probably should have checked in the first place, but this is still a reasonable restructuring.

<numeric> includes <xutility>, which is made dragging <limits> in since C++23 by #3678. I'm trying to find a way to avoid the indirect inclusion. In many cases we only need numeric_limits<I>::max(), perhaps we should invite a variable template for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
throughput Must compile faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Including <isa_availability.h> emits a non-reserved name <numeric>: Consider avoiding <limits> inclusion
4 participants