Skip to content
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

Do not include link flags for fortran linkage: Issue #3355 #3384

Merged
merged 1 commit into from
Sep 17, 2020

Conversation

jjwilke
Copy link

@jjwilke jjwilke commented Sep 14, 2020

Use generator expressions to avoid including linker flags when someone builds with Fortran.
Note:
Should be stable with CMake 3.18.
Iffy on CMake >= 3.13, but complicated to do
Does not work on 3.12. Fortran linkage will require 3.13 and above.

Fixes #3325, fixes #3355.

@dalg24
Copy link
Member

dalg24 commented Sep 15, 2020

Retest this please

Copy link
Contributor

@masterleinad masterleinad left a 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. I checked that I still see the architecture flag for a C++ target and that the minimal example in #3355 builds fine (without the architecture flag on the link line) for CMake 13 and CMake 18.

@crtrott
Copy link
Member

crtrott commented Sep 15, 2020

I assume this also means we don't get the OpenMP flag on the link line?

@crtrott
Copy link
Member

crtrott commented Sep 15, 2020

Should we error out if someone enabled CUDA Relocatable Device code?

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments: please respond, if you say we can add that later or we just have to leave that to whoever is doing the fortran thing sure. We may want to issue a warning like: "WARNING: You are using a Fortran linker, this may or may not work for a code with Kokkos dependency. Good Luck!"

@jjwilke
Copy link
Author

jjwilke commented Sep 15, 2020

I don't think I can do anything if someone overrides the LINK_LANGUAGE in a downstream project. I can't print an error message with generator expressions. At best, I could add some linker flag KOKKOS_CANT_USE_FORTRAN_LINKER_WITH_RDC that creates a somewhat meaningful error message. There's really no way to generate a warning when someone sets the LINK_LANGUAGE. I could add some documentation in a known issues section?

@jjwilke
Copy link
Author

jjwilke commented Sep 15, 2020

I assume this also means we don't get the OpenMP flag on the link line?

Correct, I am turning off all linker flags from Kokkos when Fortran is the linker. I can be more selective, but the safest and simplest thing to me seemed to be "hey, if you're linking with f90, you're totally on your own".

@crtrott
Copy link
Member

crtrott commented Sep 15, 2020

Ok please add documentation that this is an issue.

Comment on lines 248 to 256
ELSEIF(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.13")
#I can use link options
#check for CXX linkage in a super complicated way
SET(IS_STATIC $<STREQUAL:$<TARGET_PROPERTY:TYPE>,STATIC_LIBRARY>)
#I can't query linker_language on static libraries (and it doesn't matter anyway)
SET(LINK_TYPE $<${IS_STATIC}:CXX>$<$<NOT:${IS_STATIC}>:$<TARGET_PROPERTY:LINKER_LANGUAGE>>)
TARGET_LINK_OPTIONS(
${LIBRARY_NAME} PUBLIC
$<$<STREQUAL:${LINK_TYPE},CXX>:${KOKKOS_LINK_OPTIONS}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be we only support this with CMake 3.18

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will discuss this today, but I am also in favor of dropping this elseif branch and converting it back to what it was before. I.e. only support the CXX only linkage with cmake 3.18.

@crtrott crtrott merged commit 951e415 into kokkos:develop Sep 17, 2020
@dalg24 dalg24 mentioned this pull request Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants