-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[libc++][Windows] Enable thread::hardware_concurrency to support more than 64 processors #168229
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
Conversation
… than 64 processors
|
@llvm/pr-subscribers-libcxx Author: Yexuan Xiao (YexuanXiao) Changes… than 64 processors Starting with Windows 11, processes can utilize more than 64 processors by default, but GetSystemInfo can only report a maximum of 64. Starting with Windows Vista, GetActiveProcessorCount accurately retrieves the total number of processors on the current system. I’ve implemented similar improvements to Microsoft STL. The following links contain additional background information: microsoft/STL#5453, microsoft/STL#5459 (note: Reason STL uses the more complex GetLogicalProcessorInformationEx: microsoft/STL#5459 (comment).). Full diff: https://github.com/llvm/llvm-project/pull/168229.diff 1 Files Affected:
diff --git a/libcxx/src/thread.cpp b/libcxx/src/thread.cpp
index 028d36e3bfb37..e494574ec21dd 100644
--- a/libcxx/src/thread.cpp
+++ b/libcxx/src/thread.cpp
@@ -74,9 +74,7 @@ unsigned thread::hardware_concurrency() noexcept {
return 0;
return static_cast<unsigned>(result);
#elif defined(_LIBCPP_WIN32API)
- SYSTEM_INFO info;
- GetSystemInfo(&info);
- return info.dwNumberOfProcessors;
+ return static_cast<unsigned>(GetActiveProcessorCount(ALL_PROCESSOR_GROUPS));
#else // defined(CTL_HW) && defined(HW_NCPU)
// TODO: grovel through /proc or check cpuid on x86 and similar
// instructions on other architectures.
|
Thanks for these links for context, that's appreciated! For libc++, I'm not sure how many care about being able to target UWP. For llvm-mingw, I do want to support that to some extent - but there, we primarily use the strategy of linking in a static library of stubs for functions that we aren't allowed to call - https://github.com/mingw-w64/mingw-w64/tree/master/mingw-w64-libraries/winstorecompat/src. I think it should be straightforward to add emulation of Therefore, I think it should be fine to just go with this simple straightforward implementation here. |
Such an implementation done in https://sourceforge.net/p/mingw-w64/mailman/message/59262278/. So unless anyone else has a desire to target UWP with libc++, this should be fine with me. |
mstorsjo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
frederick-vs-ja
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I modified the PR title and description, which are generally used as squashed commit message, to a more conventional form.
… than 64 processors (llvm#168229) Starting with Windows 11, processes can utilize more than 64 processors by default, but GetSystemInfo can only report a maximum of 64. Starting with Windows Vista, GetActiveProcessorCount accurately retrieves the total number of processors on the current system. I’ve implemented similar improvements to Microsoft STL. The following links contain additional background information: microsoft/STL#5453, microsoft/STL#5459 (note: Reason STL uses the more complex GetLogicalProcessorInformationEx: microsoft/STL#5459 (comment).).
… than 64 processors (llvm#168229) Starting with Windows 11, processes can utilize more than 64 processors by default, but GetSystemInfo can only report a maximum of 64. Starting with Windows Vista, GetActiveProcessorCount accurately retrieves the total number of processors on the current system. I’ve implemented similar improvements to Microsoft STL. The following links contain additional background information: microsoft/STL#5453, microsoft/STL#5459 (note: Reason STL uses the more complex GetLogicalProcessorInformationEx: microsoft/STL#5459 (comment).).
|
Where is ALL_PROCESSOR_GROUPS coming from? We are getting this error in Android: |
It should be defined in |
|
It seems it is guarded by |
I see. Well In I would suggest bumping your |
|
I asked someone and it does seem we use winpthreads: https://android.googlesource.com/toolchain/llvm_android/+/mirror-goog-main-llvm-toolchain-source/do_build.py#284. People are out but I will discuss bumping up to 0x601 when they are back |
… than 64 processors (llvm#168229) Starting with Windows 11, processes can utilize more than 64 processors by default, but GetSystemInfo can only report a maximum of 64. Starting with Windows Vista, GetActiveProcessorCount accurately retrieves the total number of processors on the current system. I’ve implemented similar improvements to Microsoft STL. The following links contain additional background information: microsoft/STL#5453, microsoft/STL#5459 (note: Reason STL uses the more complex GetLogicalProcessorInformationEx: microsoft/STL#5459 (comment).).
Starting with Windows 11, processes can utilize more than 64 processors by default, but GetSystemInfo can only report a maximum of 64. Starting with Windows Vista, GetActiveProcessorCount accurately retrieves the total number of processors on the current system. I’ve implemented similar improvements to Microsoft STL. The following links contain additional background information: microsoft/STL#5453, microsoft/STL#5459 (note: Reason STL uses the more complex GetLogicalProcessorInformationEx: microsoft/STL#5459 (comment).).