-
Notifications
You must be signed in to change notification settings - Fork 407
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
Export the flags in KOKKOS_AMDGPU_OPTIONS when using Trilinos #5528
Conversation
@@ -228,6 +229,9 @@ IF (KOKKOS_HAS_TRILINOS) | |||
# we have to match the annoying behavior, also we have to preserve quotes | |||
# which needs another workaround. | |||
SET(KOKKOS_COMPILE_OPTIONS_TMP) | |||
IF (KOKKOS_ENABLE_HIP) | |||
LIST(APPEND KOKKOS_COMPILE_OPTIONS ${KOKKOS_AMDGPU_OPTIONS}) |
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.
Can you confirm that is the right thing to do
CUDA seems to be using KOKKOS_ALL_COMPILE_OPTIONS
Line 246 in 1a63570
LIST(APPEND KOKKOS_ALL_COMPILE_OPTIONS ${KOKKOS_CUDA_OPTIONS}) |
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.
Yes, I know that CUDA does it differently but the current PR is correct for hip
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 this has something to do with whether its ; separated list or not or so?
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 am asking because I saw
INTERFACE_COMPILE_OPTIONS "@KOKKOS_ALL_COMPILE_OPTIONS@" |
Should we comment then to make sure it does not look like a mistake?
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.
KOKKOS_COMPILER_OPTIONS is appended to KOKKOS_ALL_COMPILE_OPTIONS just a few lines below.
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 I see
Line 244 in 1a63570
LIST(APPEND KOKKOS_ALL_COMPILE_OPTIONS ${KOKKOS_COMPILE_OPTIONS}) |
I don't even want to look why we would need both
KOKKOS_ALL_COMPILE_OPTIONS
and KOKKOS_COMPILER_OPTIONS
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.
Any reason not to just use COMPILER_SPECIFIC_FLAGS
? That doesn't seem to require doing something special for Trilinos
.
It does not work. I also though it would be cleaner but then I would need to rewrite more of the build system which I don't want to do. |
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.
Let's clean the build system another time. 🙂
With this PR, we export the flags stored in
KOKKOS_AMDGPU_OPTIONS
when using Trilinos.Related to #5505