TurbSim: OpenMP parallelization in factorization and FFT #3020
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Very cludgy mess. Seems to work locally though.
andrew-platt
force-pushed
the
f/TurbSim_OpenMP
branch
from
f090d87 to
1637f4a
Compare
The TRH_in in the SHARED was causing memory issues. Removing that makes things identical now.
No need for the inner loop.
…se THREADPRIVATE and COPYIN to create the necessary copies of TRH array in CalcFourierCoeffs_IEC instead of doing it manually
…hen not using OpenMP or explicitly setting the BLAS vendor. This should reduce the chances of slowdowns from calling the MKL with many small calculations trying to distribute it over many threads.
andrew-platt
commented
Nov 24, 2025
andrew-platt
commented
Nov 25, 2025
Collaborator
Author
andrew-platt
left a comment
andrew-platt
left a comment
There was a problem hiding this comment.
@deslaughter's changes are great - a much better method than what I proposed.
bjonkman
reviewed
Nov 29, 2025
| CONTAINS | ||
| SUBROUTINE Cleanup() | ||
|
|
||
| IF ( ALLOCATED( Dist ) ) DEALLOCATE( Dist ) |
Contributor
There was a problem hiding this comment.
In this subroutine, we are allocating memory, but with removing the cleanup() reoutine, we are not explicitly deallocating it anymore. I know this routine is called only once, so maybe isn't a big concern, but I've seen this cause memory leaks or allocation errors because the array is already allocated when the routine gets called a second time. Thoughts?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Needs some review and testing.
Feature or improvement description
TurbSim has been single-threaded forever. This PR adds parallelization through OpenMP and should in theory run faster as a result.
If OpenMP is enabled at compile time, parallelization for MKL will be automatically turned off. We saw this the MKL parallelization is an issue in #3018 when the MKL would try to parallelize some tiny matrix manipulation incurring massive overhead costs.
Still need to extend to the following routines:
CalcFourierCoeffs_APICalcFourierCoeffs_GeneralCalcFourierCoeffs_NoneRelated issue, if one exists
#3018
Impacted areas of the software
TurbSim only
Additional supporting information
APRIVATE(<allocatable_array>)in a!$OMP PARALLEL DOpragma doesn't always allocate the copy of that array. This leads to segfaults. I have messy workarounds for this. There may be a better way to do it, but I have no idea if the ROCM Flang compiler will be able to handle anything more advanced.THREADPRIVATEwithCOPYINworks better than the original solutionTest results, if applicable
No test results should change.