Adjusted OpenMP flags #431
Conversation
* bumped min. required CMake version to 3.10 to guarantee CMake support of OpenMP * removed custom FindOpenMP.cmake from SeisSol * removed hardcoded openmp compiler and linker flags
ravil-mobile
force-pushed
the
ravil/openmp
branch
2 times, most recently
from
bf40307
to
1835acf
Compare
* split compilation of generated code with the main seissol source files * added OpenMP flags through imported target * rearrange targets
ravil-mobile
force-pushed
the
ravil/openmp
branch
from
1835acf
to
e27a3fc
Compare
CMakeLists.txt
Outdated
| target_link_libraries(SeisSol-lib PUBLIC OpenMP::OpenMP_Fortran) | ||
|
|
||
| find_package(OpenMP_Offloading QUIET) | ||
| # Note: there is a conflict between OpenMP offloading and libsxmm |
There was a problem hiding this comment.
Maybe add a comment here why that is
There was a problem hiding this comment.
Added
# Note: there may be a conflict between OpenMP offloading and libsxmm.
# A collision can happen due to -march flags for CPUs and OpenMP GPU target
# (e.g. with clang -Xopenmp-target -march=sm_75). Therefore, we separate
# compilations for the source and generated codes and apply different
# imported OpenMP targets to them.
CMakeLists.txt
Outdated
|
|
||
| # Note: we need to provide a support for Eigen backend | ||
| # for both gencode and SeisSol | ||
| target_include_directories(SeisSol-gencode PUBLIC |
There was a problem hiding this comment.
Hm, why is this PUBLIC? Shouldn't this be private?
There was a problem hiding this comment.
So, as the comment says we need eigen (and yateto) headers for the code generated by yateto (in case of Eigen backend) and SeisSol-lib. If it is PRIVATE then the includes will be applied only for SeisSol-gencode target.
There was a problem hiding this comment.
But aren't we using the headers anyway for SeisSol-lib? I'm not sure whether doing it this way might hide some dependencies. Probably doesn't matter much.
There was a problem hiding this comment.
Yes, we are using. I mean the path to eigen will be applied to SeisSol-gencode and everything which depends on SeisSol-gencode. In our case, it is SeisSol-lib and so on (SeisSol-bin, SeisSol-proxy, tests, etc.).
| ) | ||
|
|
||
| target_include_directories(SeisSol-gencode PUBLIC | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/submodules/yateto/include |
There was a problem hiding this comment.
you're right. I will remove it
cmake/FindOpenMP_Offloading.cmake
Outdated
|
|
||
| cmake_push_check_state() | ||
|
|
||
| set(_PARENT_CMAKE_CXX_STANDARD ${CMAKE_CXX_STANDARD}) |
There was a problem hiding this comment.
Why is this needed? We are using CMAKE_CXX_STANDARD 17 anyway?
There was a problem hiding this comment.
I guess I can remove CMAKE_CXX_STANDARD from this file. I started with a copy of FindFILESYSTEM.cmake file and forgot to remove it.
In case of FindFILESYSTEM.cmake, in general, one doesn't know whether CMAKE_CXX_STANDARD 17 has already been set. It was just a generalization in case of somebody decides to re-use the file for other sub-projects.
* enforce to build SeisSol-gencode before SeisSol-device-lib
This comment has been minimized.
This comment has been minimized.
ravil-mobile
force-pushed
the
ravil/openmp
branch
from
3cab9b9
to
bf097fe
Compare
to guarantee CMake support of OpenMP
Tested with gcc, intel and clang and it worked well for both c++ and fortran compilers