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

[OpenMP][test][VE] Limit the number of threads to create #65873

Closed
wants to merge 1 commit into from
Closed

[OpenMP][test][VE] Limit the number of threads to create #65873

wants to merge 1 commit into from

Conversation

kaz7
Copy link
Contributor

@kaz7 kaz7 commented Sep 10, 2023

VE supports up to 64 threads per a VE process. So, we limit the number of threads defined by KMP_MAX_NTH. We also modify the __kmp_sys_max_nth initialization to use KMP_MAX_NTH as a limit. We also modify a test program itself.

@kaz7 kaz7 requested a review from a team September 10, 2023 03:43
@llvmbot
Copy link
Member

llvmbot commented Sep 10, 2023

@llvm/pr-subscribers-openmp

Changes

Limit the number of threads to create in a test program because VE's pthread_create supports only 64 threads at a maximum.

Full diff: https://github.com/llvm/llvm-project/pull/65873.diff

1 Files Affected:

  • (modified) openmp/runtime/test/atomic/omp-atomic-compare-signedness.c (+5)
diff --git a/openmp/runtime/test/atomic/omp-atomic-compare-signedness.c b/openmp/runtime/test/atomic/omp-atomic-compare-signedness.c
index 41c0d5617a7704c..ca37d717a7f8918 100644
--- a/openmp/runtime/test/atomic/omp-atomic-compare-signedness.c
+++ b/openmp/runtime/test/atomic/omp-atomic-compare-signedness.c
@@ -11,7 +11,12 @@
 // UNSUPPORTED: gcc
 
 // High parallelism increases our chances of detecting a lack of atomicity.
+#ifdef __ve__
+// VE's pthread_create supports 64 threads at a maximum.
+#define NUM_THREADS_TRY 64
+#else
 #define NUM_THREADS_TRY 256
+#endif
 
 #include 
 #include 

Copy link

@sfc9982 sfc9982 left a comment

Choose a reason for hiding this comment

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

nice feature

@@ -11,7 +11,12 @@
// UNSUPPORTED: gcc

// High parallelism increases our chances of detecting a lack of atomicity.
#ifdef __ve__
// VE's pthread_create supports 64 threads at a maximum.
Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case, then you want to set the limit (such as __kmp_sys_max_nth) in kmp.h as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Modifying initial value for __kmp_sys_max_nth is not enough for our cases since __kmp_runtime_initialize is implemented assuming NPTL which assumes infinity number of threads. I modify both KMP_MAX_NTH and the mechanism in __kmp_runtime_initialize.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to split the patch into two: one is about test, as the previous version, and one for the changes in the header, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created #66729 . I'm not sure whether I create another patch for test or not. Test itself passes after applying #66729 with following warnings. I can say this is good regression tests for #66729 . I also can say it's good to change test too for let ppl know this limitation. Do you have any opinion?

OMP: Warning #96: Cannot form a team with 256 threads, using 64 instead.
OMP: Hint Consider unsetting KMP_DEVICE_THREAD_LIMIT (KMP_ALL_THREADS), KMP_TEAMS_THREAD_LIMIT, and OMP_THREAD_LIMIT (if any are set).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to hear the test can pass after #66729. I'm more inclined to leave the test alone since we don't know what users can really do and the number looks totally reasonable from OpenMP's perspective. It is just our implementation can't handle it correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That make sense too. I'll leave the test as is. Thank you for your advice.

VE supports up to 64 threads per a VE process.  So, we limit the number
of threads defined by KMP_MAX_NTH.  We also modify the __kmp_sys_max_nth
initialization to use KMP_MAX_NTH as a limit.  We also modify test program
itself.
@kaz7
Copy link
Contributor Author

kaz7 commented Sep 19, 2023

Closing this ticket to modify the test. We change the openmp max num threads for VE in #66729 .

@kaz7 kaz7 closed this Sep 19, 2023
@kaz7 kaz7 deleted the main-openmp-test-reduce-num-threads branch September 19, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants