-
Notifications
You must be signed in to change notification settings - Fork 32
Improves test throughput, especially for threaded tests #1749
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
base: develop
Are you sure you want to change the base?
Changes from all commits
4e0b17d
1b593b8
144380b
414e1c2
2c97d4f
807cd57
d1d1d39
a10bf77
e3e5fa3
db0ae2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,26 +73,36 @@ if(AXOM_ENABLE_TESTS) | |
| # Run the core array performance example on N ranks for each enabled policy | ||
| set(_policies "seq") | ||
| if(RAJA_FOUND) | ||
| blt_list_append(TO _policies ELEMENTS "omp" IF AXOM_ENABLE_OPENMP) | ||
| blt_list_append(TO _policies ELEMENTS "omp" IF AXOM_ENABLE_OPENMP) | ||
| blt_list_append(TO _policies ELEMENTS "cuda" IF AXOM_ENABLE_CUDA) | ||
| blt_list_append(TO _policies ELEMENTS "hip" IF AXOM_ENABLE_HIP) | ||
| blt_list_append(TO _policies ELEMENTS "hip" IF AXOM_ENABLE_HIP) | ||
| endif() | ||
| foreach(_pol ${_policies}) | ||
| # sets the number of threads for the omp policy; leave empty for non-omp policies | ||
| set(_num_threads) | ||
| if(_pol STREQUAL "omp") | ||
| set(_num_threads ${AXOM_TEST_NUM_OMP_THREADS}) | ||
| endif() | ||
|
|
||
| axom_add_test( | ||
| NAME "core_array_perf_1d_${_pol}" | ||
| COMMAND core_array_perf_ex --policy ${_pol} --shape 1200000 -r 10) | ||
| NAME core_array_perf_1d_${_pol} | ||
| COMMAND core_array_perf_ex --policy ${_pol} --shape 1200000 -r 10 | ||
| NUM_OMP_THREADS ${_num_threads}) | ||
|
Comment on lines
+81
to
+90
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: This PR relies on allowing empty arguments to |
||
| axom_add_test( | ||
| NAME "core_array_perf_2d_${_pol}" | ||
| COMMAND core_array_perf_ex --policy ${_pol} --shape 1000 1200 -r 10) | ||
| NAME core_array_perf_2d_${_pol} | ||
| COMMAND core_array_perf_ex --policy ${_pol} --shape 1000 1200 -r 10 | ||
| NUM_OMP_THREADS ${_num_threads}) | ||
| axom_add_test( | ||
| NAME "core_array_perf_3d_${_pol}" | ||
| COMMAND core_array_perf_ex --policy ${_pol} --shape 100 100 120 -r 10) | ||
| NAME core_array_perf_3d_${_pol} | ||
| COMMAND core_array_perf_ex --policy ${_pol} --shape 100 100 120 -r 10 | ||
| NUM_OMP_THREADS ${_num_threads}) | ||
| if(NOT RAJA_FOUND) | ||
| # RAJA provides support for up to 3D tiled nested loops, | ||
| # so run 4D test only for non-RAJA builds. | ||
| axom_add_test( | ||
| NAME "core_array_perf_4d_${_pol}" | ||
| COMMAND core_array_perf_ex --policy ${_pol} --shape 50 20 30 6 ) | ||
| NAME core_array_perf_4d_${_pol} | ||
| COMMAND core_array_perf_ex --policy ${_pol} --shape 50 20 30 6 | ||
| NUM_OMP_THREADS ${_num_threads}) | ||
| endif() | ||
| endforeach() | ||
| endif() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,9 +140,9 @@ if (AXOM_ENABLE_OPENMP AND RAJA_FOUND) | |
|
|
||
| foreach( test_suite ${core_openmp_tests} ) | ||
| get_filename_component( test_suite ${test_suite} NAME_WE ) | ||
| axom_add_test( NAME ${test_suite}_openmp | ||
| COMMAND core_openmp_tests --gtest_filter=${test_suite}* | ||
| NUM_OMP_THREADS 24 ) | ||
| axom_add_test( NAME ${test_suite}_openmp | ||
| COMMAND core_openmp_tests --gtest_filter=${test_suite}* | ||
| NUM_OMP_THREADS ${AXOM_TEST_NUM_OMP_THREADS} ) | ||
| endforeach() | ||
| endif() | ||
|
|
||
|
|
@@ -183,14 +183,16 @@ axom_add_executable(NAME utils_annotations_test | |
|
|
||
| foreach(_mode none report counts gputx) | ||
| set(_test_name utils_annotations_${_mode}_test) | ||
|
|
||
| set(_num_ranks 1) | ||
| if(AXOM_ENABLE_MPI) | ||
| axom_add_test(NAME ${_test_name} | ||
| COMMAND utils_annotations_test -m ${_mode} | ||
| NUM_MPI_TASKS 2) | ||
| else() | ||
| axom_add_test(NAME ${_test_name} | ||
| COMMAND utils_annotations_test -m ${_mode}) | ||
| set(_num_ranks 2) | ||
| endif() | ||
|
Comment on lines
186
to
193
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: A recent BLT PR allows for |
||
|
|
||
| axom_add_test(NAME ${_test_name} | ||
| COMMAND utils_annotations_test -m ${_mode} | ||
| NUM_MPI_TASKS ${_num_ranks}) | ||
|
|
||
| set_property(TEST ${_test_name} | ||
| APPEND PROPERTY ENVIRONMENT "CALI_LOG_VERBOSITY=2") | ||
| endforeach() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Minor nit. I think it's better to be consistent with the
OMP_NUM_THREADSenv variable or OpenMP library function with a similar name. Here and throughout.Also, the terms
ranksandtasksare used interchangeably as they relate to MPI. Should that be made consistent also?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.
+1 for consistency, but it might be tricky here.
The keywords/parameters for
blt_add_testareNUM_MPI_TASKSandNUM_OMP_THREADS, while the ENV variable for OpenMP isOMP_NUM_THREADS. It's not clear that renaming the blt parameters would be appropriate or an improvement, and would affect all blt users.What do you suggest?
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.
Aargh--- naming is important and consistency is a desirable quality. I would say, let's match
blt_add_test, and in any discussion let's point out the discrepancy to OpenMP environment variables.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.
Probably best to discuss with Chris and others. I would defer to OpenMP which is a common, well-known standard, whereas BLT it not.
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.
I think it would be confusing to switch it. This is an existing macro that's been in the code for a long time and is entirely Axom developer focused.
We're calling
axom_add_test()which directly callsblt_add_test()and all other keywords/parameters are the same.Uh oh!
There was an error while loading. Please reload this page.
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.
My vote is to match BLT since it is influencing a BLT macro. The reason it is named that way is because it follows the pattern of the BLT MPI variable which was created long before the OpenMP parameter which was before I knew that
OMP_NUM_THREADSenvironment variable existed.