-
Notifications
You must be signed in to change notification settings - Fork 577
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
Kokkos: Don't let Kokkos set CMAKE_CXX_FLAGS #12572
Conversation
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: masterleinad |
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 agree, we should not be expecting CMAKE_CXX_FLAGS
to be passed along to downstream CMake customers who pull in Kokkos (or any Trilinos package) with find_package(<upstreamPkg> ...)
. Instead, critical compiler options should be propagated using the INTEFACE_COMPILE_OPTIONS
target property using target_link_libraries(<dowsntreamTarget> <INTERFACE|PUBLIC|PRIVIATE> <upstreamTarget>)
.
But then why not also remove setting CMAKE_CXX_FLAGS
as a local var which will hide CMAKE_CXX_FLAGS
set as a cache var and not allow the user to tweak any of these flags? I think this should be addressed before merging this PR (see below).
packages/kokkos/CMakeLists.txt
Outdated
# In ProjectCompilerPostConfig.cmake, we capture the "global" flags Trilinos wants in | ||
# TRILINOS_TOPLEVEL_CXX_FLAGS | ||
SET(CMAKE_CXX_FLAGS "${TRILINOS_TOPLEVEL_CXX_FLAGS} ${KOKKOSCORE_CXX_FLAGS}" PARENT_SCOPE) | ||
SET(CMAKE_CXX_FLAGS "${TRILINOS_TOPLEVEL_CXX_FLAGS} ${KOKKOSCORE_CXX_FLAGS}") |
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.
Why is CMAKE_CXX_FLAGS
still being set as a local var? This line should either be removed or should be changed to:
SET(CMAKE_CXX_FLAGS "${TRILINOS_TOPLEVEL_CXX_FLAGS} ${KOKKOSCORE_CXX_FLAGS} ${CMAKE_CXX_FLAGS}")
so that it will allow the user to override any of these flags (and therefore negate them if needed) or:
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${TRILINOS_TOPLEVEL_CXX_FLAGS} ${KOKKOSCORE_CXX_FLAGS} ")
so that the user's options don't override these Kokkos-set options.
Simply setting CMAKE_CXX_FLAGS
as a local variable (and hiding the CMake cache var of the same name) is a big CMake no-no (see section 16.6.3. "Prefer Appending Over Replacing Flags" in the book "Professional CMake: 16th Edition").
But if Kokkos is setting critical flags through target_compile_options(Kokkos::<kokkoslib> ...)
, then why bother touching the CMAKE_CXX_FLAGS
var at all? Just set those compiler options in the flags passed to target_compile_options(Kokkos::<kokkoslib> ...)
and then let the user add additional options in the cache var CMAKE_CXX_FLAGS
.
NOTE: Removing or changing that line of code could technically break backward compatibility but it seems unlikely that anyone will get bit by this (or problems will be rare and should be easy to recover from).
NOTE: There is a long discussion of these issues with Brad King (of which you were @mentioned) in:
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.
This code path is only used for Trilinos
anyway and I tried to be conservative. Hopefully, we can move forward with kokkos/kokkos#6164 soon and clean up the interaction between Trilinos
and Kokkos
some more.
I'm happy to give it a shot to remove the whole block and all related code already.
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.
@masterleinad, if you want to defer final cleanup, then that is fine and we can go ahead and merge this. But given problems with the PR builds (see #12571), there is no hurry. I will go ahead and remove my "request changes" and you can decide what you want to do with this.
Status Flag 'Pull Request AutoTester' - Error: Jenkins Jobs - A user has pushed a change to the PR before testing completed. NEW EVENT 'committed', ID C_kwDOAsJyMdoAKDdmNTAxMTRhZTNhODNhYWU1MTUzMmQ2YzY4N2E3YjViNDlmMTM1ZDI... The Jenkins Jobs will be shutdown; Testing of this PR must occur again. |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
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.
This cleanup is not critical and since there is a more significant refactoring of the Kokkos build under Trilinos coming (see kokkos/kokkos#6164), I will defer to others if to address the last CMAKE_CXX_FLAGS issue in this PR or not.
FYI: Don't bother putting on is resolved. |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Can we retest this and come to a conclusion if we want both commits or only the first one? It seems this is also causing problems for |
@masterleinad, let's pull the trigger on the suggestions in above in a new commit and then trigger the retest? |
7f50114
to
a1f7597
Compare
@bartlettroscoe I pushed a more conservative version than I had before (in https://github.com/masterleinad/Trilinos/pull/new/more_sycl_fixes_backup as backup now) that avoids setting |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
@masterleinad we'll need these changes applied to the kokkos repo, as well as applied to the release-candidate-4.2.01 branch if this PR makes it into Trilinos before the patch release (to avoid clobber) |
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.
Looks good to me. Rip it out. Less is more :-)
STRING(REPLACE ";" " " KOKKOSCORE_CUDA_OPTIONS "${KOKKOS_CUDA_OPTIONS}") | ||
FOREACH(CUDAFE_FLAG ${KOKKOS_CUDAFE_OPTIONS}) | ||
SET(KOKKOSCORE_CUDAFE_OPTIONS "${KOKKOSCORE_CUDAFE_OPTIONS} -Xcudafe ${CUDAFE_FLAG}") | ||
LIST(APPEND KOKKOS_ALL_COMPILE_OPTIONS -Xcudafe ${CUDAFE_FLAG}) |
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.
Removing this block of code seems to have the side effect that the cuda fe flags are no longer passed to Trilinos through KOKKOS_ALL_COMPILE_OPTIONS. May be good to doublecheck if that’s intended.
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.
You're right. For now, it's better not to modify KOKKOS_ALL_COMPILE_OPTIONS
. I pushed a corresponding commit.
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.
OK!
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.
The handling of KOKKOS_ALL_COMPILE_OPTIONS
is a bit confusing to me so I don't understand the logic here. From what I can tell, these options are written to a configured file KokkosTrilinosConfig.cmake
and added to the Kokkos::kokkos
target using target_compiler_options()
. Then that file gets read in and written to the end of the generated file KokkosConfig.cmake
which gets installed. Why not just directly add those compile options to the individual Kokkos libraries using target_compiler_options()
? Then those options would be carried along with the direct Kokkos libraries no matter how they are used.
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 agree but KOKKOS_ALL_COMPILE_OPTIONS
doesn't cause the issue we are trying to fix here and we wanted to keep the changes here minimal.
Note that all this logic is just for Trilinos
. A standalone Kokkos
doesn't even go through these lines of code.
With the refactoring in #11779 we should eliminate all the weirdness we had in Kokkos just to make Trilinos work.
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
What do the test failures look like? |
@masterleinad, the main error was building the object file:
in the build:
which was:
The test Phalanx_experimental_mdfield_refactor_MPI_1 also segfaulted in the build PR-12572-test-rhel7_sems-intel-2021.3-sems-openmpi-4.0.5_release-debug_shared_no-kokkos-arch_no-asan_no-complex_fpic_mpi_no-pt_no-rdc_no-uvm_deprecated-off_no-package-enables-1708 (ascic114) but I would guess that is unrelated. |
These were fixed in kokkos/kokkos#6510 which is not in 4.2.00 |
I'm surprised that these problems only show up in this pull request. Are pull requests tested rebased on top of |
2d2078e
to
84fdfd5
Compare
I rebased and cherry-picked kokkos/kokkos#6510 so we can retest. The best solution would be to merge this as part of the upcoming Kokkos patch release anyway. |
@masterleinad, that is a test file and that only gets built if enabling all of the Trilinos packages or when Kokkos is changed. Most PRs builds would not enable the Kokkos tests because they don't contain changes that could break Kokkos. This is why I argue that Trilinos needs a post-merge CI build that will catch issues like this. |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: masterleinad |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
What's the failure this time? |
@masterleinad looks like some of the jobs did not complete, related to #12696 and possibly backlog of jobs waiting in the queue when the GPU nodes came back online. Retesting |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: masterleinad |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
Looks like the
No failures reported in the other builds |
We merged this one on the |
@trilinos/kokkos
Motivation
Kokkos
is currently settingCMAKE_CXX_FLAGS
forTrilinos
which is problematic.target_link_libraries(target Kokkos::kokkos)
(or an alias forKokkos::kokkos
) since the compile optionfsycl-targets=spir64_gen
can only be used if-Xsycl-target-backend
is used as linker flag.CMAKE_CXX_FLAGS
forces compile options on all targets that don't need it overriding possibly more suitable flags.The lines modified here were last touched in kokkos/kokkos#2660 suggesting that
Trilinos
was relying onCMAKE_CXX_FLAGS
being set byKokkos
at least at some point. I assume this has become obsolete by usingtarget_link_libraries
more consistently. @bartlettroscoe does that match your expectations?Note that the changes here are also included in #11779 which makes
Kokkos
aTriBits
-compatible rawCMake
project snapshotted inTrilinos
.Related Issues
Testing
Compiling
Trilinos
withKokkos+SYCL
.