Skip to content

Concentrated header for internal bit utilities#3721

Merged
StephanTLavavej merged 10 commits intomicrosoft:mainfrom
frederick-vs-ja:bit-utils
Jun 15, 2023
Merged

Concentrated header for internal bit utilities#3721
StephanTLavavej merged 10 commits intomicrosoft:mainfrom
frederick-vs-ja:bit-utils

Conversation

@frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented May 20, 2023 •
edited
Loading

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>.

@frederick-vs-ja
Concentrated header for internal bit utilities
c168cef 
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
StephanTLavavej
StephanTLavavej approved these changes Jun 12, 2023
View reviewed changes
stl/inc/__msvc_bit_utils.hpp
#endif // _HAS_NEON_INTRINSICS

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

@StephanTLavavej StephanTLavavej Jun 12, 2023

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.

Sign in to view
@StephanTLavavej StephanTLavavej self-assigned this Jun 14, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 14, 2023

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

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 14, 2023

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.

StephanTLavavej
StephanTLavavej approved these changes Jun 15, 2023
View reviewed changes
stl/inc/__msvc_bit_utils.hpp
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

@StephanTLavavej StephanTLavavej Jun 15, 2023

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

StephanTLavavej commented Jun 15, 2023

Thanks for this major header restructuring! 🛠️ 🎉 😸

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 15, 2023

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

frederick-vs-ja commented Jun 15, 2023

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.

This was referenced Jun 20, 2023
Closed
Merged
@StephanTLavavej StephanTLavavej mentioned this pull request Jul 21, 2023
Merged
@lhecker lhecker mentioned this pull request Aug 10, 2023
Merged
lhecker added a commit to microsoft/terminal that referenced this pull request Aug 11, 2023
@lhecker
Fix compilation with Visual Studio 17.8 (#15819)
e9c8391 
This broke with microsoft/STL#3721
It's a minor issue and a minor fix. :)
@cpplearner cpplearner mentioned this pull request Mar 4, 2024
Merged
@jovibor jovibor mentioned this pull request Oct 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+1 more reviewer

@barcharcraz barcharcraz barcharcraz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

throughput Must compile faster

Projects

None yet

Milestone

No milestone

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

@frederick-vs-ja @StephanTLavavej @barcharcraz @CaseyCarter