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

Minor build fixes in cmake #55

Merged
merged 4 commits into from
Apr 24, 2019

Conversation

VileLasagna
Copy link
Contributor

Three minor cmake fixes in hipsycl itself:

1 - Explicitly set CMAKE_CXX_STANDARD to 14, in case it doesn't get automagically set

2 - Set and forwarded CMAKE_CUDA_HOST_COMPILER and CMAKE_CUDA_STANDARD to the CUDA target if building with NVCC, as those don't always get correctly inherited to match the C++ side of the build

3 - Probed the system for the ROCM_PATH variable and used it, if present, to add ROCM's include path to the build. I myself had always been adding the full path manually, but ROCM sets this on a file in profile.d when it install itself, so probing this env variable is an option

None of these changes should break pre-existing builds. The one that could would be no1 if the compiler doesn't support it but it seems to me SYCL doesn't even build with a standard lower than 14 due to lambda shenanigans

@VileLasagna VileLasagna mentioned this pull request Apr 22, 2019
@illuhad
Copy link
Collaborator

illuhad commented Apr 22, 2019

Thank you! :) However, it seems that this PR breaks the Travis CI build (at least with clang-cuda), interestingly with the same error as reported in #50. I've just rechecked and it builds fine without this PR. Could you please investigate? Does this PR depend on PR #40 ?

@VileLasagna
Copy link
Contributor Author

It should build straight off of current master. Can you give me the console output of your CI so I can have a look?

@illuhad
Copy link
Collaborator

illuhad commented Apr 22, 2019

See here. For testing, you can use the Dockerfiles or singularity definition files in install (replace repository with your fork and checkout your branch), as the CI just tries to build those.

@VileLasagna
Copy link
Contributor Author

That's interesting. I run into this specific issue myself quite a bit with clang and nvcc. The workaround is to pass -D__STRICT_ANSI__ into CXX_FLAGS. Curious it showed up only now in that build. I'll have a play but my guess is that the culprit would be me enforcing CXX_STD to 14. If it just works otherwise in CI I might just revert that one commit.

@VileLasagna
Copy link
Contributor Author

It indeed was the CXX standard... kind of curious that it seemed I bumped into needing it on my side. Good to come in contact with the docker stuff to check before pushing though. I've just pushed a revert commit but can, if you'd prefer to have a cleaner tree, do some evil rebasing trickery and get rid of the commit(s) altogether

@psalz
Copy link
Member

psalz commented Apr 23, 2019

Without having looked at the CI output our tried building this myself, it might be due to CMake defaulting to GNU standards (i.e., gnu++14) on Linux. Try setting set_property(TARGET <target> PROPERTY CXX_EXTENSIONS OFF) - at least that's what I previously had to do for a similar sounding issue.

@illuhad
Copy link
Collaborator

illuhad commented Apr 24, 2019

Thanks for all the ideas. I think we should try getting C++14 enabled by default because of the feedback from issue #56, where a missing -std=c++14 caused compilation failures. @VileLasagna, could you please have a look at @psalz's proposals?

@VileLasagna
Copy link
Contributor Author

Sure thing. Will try and massage that into conformity

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

3 participants