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

nvcc_wrappers and c++ 14 #2035

Closed
bathmatt opened this issue Mar 15, 2019 · 21 comments
Closed

nvcc_wrappers and c++ 14 #2035

bathmatt opened this issue Mar 15, 2019 · 21 comments
Assignees
Labels
Enhancement Improve existing capability; will potentially require voting

Comments

@bathmatt
Copy link

I'd like to modify nvcc_wrapper to translate c++1z to c++14 as nvcc supports the latter but not the former.. cmake is puking on this in configure on some things....

Thoughts?

@dhollman
Copy link

C++1z should correspond to C++17, not C++14. In older compilers, C++14 was referred to as C++1y. The flags C++1z and C++14 should not be interchangeable; they mean different things.

@nmhamster
Copy link
Contributor

@bathmatt - can you override this in cmake so you get the C++14 flags (everywhere)? I agree with David, this could create a world of pain as certain things get enabled that aren’t really supported.

@bathmatt
Copy link
Author

I'm sure I can fix it in cmake, no idea how as cmake is a giant mass of unknown.. Any ideas?

I'm asking for c++14 and cmake is doing this
target_compile_features(${DETECTION_LIBRARY} INTERFACE cxx_std_14)

but ...

Run Build Command:"/usr/bin/gmake" "cmTC_76013/fast"
/usr/bin/gmake -f CMakeFiles/cmTC_76013.dir/build.make CMakeFiles/cmTC_76013.dir/build
gmake[1]: Entering directory `/home/mbetten/tmp/f/CMakeFiles/CMakeTmp'
Building CXX object CMakeFiles/cmTC_76013.dir/feature_tests.cxx.o
/home/mbetten/bin/nvcc_wrapper    -std=c++1z -o CMakeFiles/cmTC_76013.dir/feature_tests.cxx.o -c /home/mbetten/tmp/f/CMakeFiles/feature_tests.cxx
nvcc fatal   : Value 'c++1z' is not defined for option 'std'
gmake[1]: *** [CMakeFiles/cmTC_76013.dir/feature_tests.cxx.o] Error 1
gmake[1]: Leaving directory `/home/mbetten/tmp/f/CMakeFiles/CMakeTmp'

@bradking
Copy link
Contributor

The problem is that nvcc_wrapper is pretending to be a C++ compiler when it is actually a CUDA compiler. In particular, CMake identifies it as whatever C++ compiler nvcc launches internally and has no knowledge that anything special is happening. In this case nvcc_wrapper is identified as a version of the GNU compiler that supports the -std=c++1z flag. There is no such thing as a g++ 5.5 that does not support -std=c++1z. CMake expects that flag to work and passes it as part of checking that the compiler works as expected. Since the flag does not work CMake gives up assuming something is wrong with the compiler.

@crtrott
Copy link
Member

crtrott commented Mar 18, 2019

Why does make add c++1z when asked for C++14? That doesn't make any sense. c++1z is the "I kinda support C++17 features, but that standard is not yet ratified" flag.

@crtrott
Copy link
Member

crtrott commented Mar 18, 2019

Also nvcc_wrapper is not pretending anything, it is not a compiler, it just reorders flags and adds a couple. The fact that it is identified as a C++ compiler is because cmake thinks all cpp files are C++ files. In our case all cpp files are CUDA files.

@bradking
Copy link
Contributor

Why CMake treats nvcc_wrapper as a C++ comiler

CMake is being told to use nvcc_wrapper to compile C++ sources (perhaps through OMPI_CXX). In that sense users are pretending nvcc_wrapper is a C++ compiler, and CMake treats it as such. CMake uses the compiler's preprocessor to identify the compiler vendor and version. Since nvcc is not a C++ compiler, it is not considered as a possible identity. As a result CMake identifies nvcc_wrapper as whatever system compiler that nvcc runs internally, e.g. GNU 5.5.

Why CMake runs the compiler with -std=c++1z

CMake has a step where it detects what features are provided by the compiler at each standard level (98, 11, 14, 17, etc.) so that it can honor target_compile_features requests for features requiring that level. To do this, CMake tries running the compiler with the flag for each standard level, and for GNU 5.5 CMake knows that the flag for C++17 is -std=c++1z. This works well so long as the compiler really supports all the flags that it should based on its identity, but nvcc_wrapper does not provide this flag.

Solution

We're looking at changing the way the above logic works in CMake to avoid the C++17 (and above) flag checks altogether. Then -std=c++1z will only be passed to the compiler if the project asks for C++17 support.

FWIW, CMake does have first-class CUDA support that does not need nvcc_wrapper at all and can directly generate build rules using nvcc.

@dhollman
Copy link

@crtrott I’m having a talk with CMake Rob about this. He’s here at GTC and has some ideas about how to fix this. We should set something up

@crtrott
Copy link
Member

crtrott commented Mar 18, 2019

Sure I am just finishing up some stuff in my hotel room. We can meet later if you guys are interested. There are some issues around the question of do now all our users need to set CMAKE_CXX_FLAGS and CMAKE_CUDA_FLAGS? CMAKE_CXX_FLAGS wouldn't actually ever be used as far as I understand since all our C++ files are CUDA files. How do the MPI Wrappers fit into the picture? The primary reason for nvcc_wrapper originally was that mpicxx couldn't be used as the host compiler for nvcc, and nvcc couldn't be used as the CXX compiler under mpicxx.

How does linking work with RDC on, where we need to have both MPI stuff linked in and NVCC must be the linker etc.

@bradking
Copy link
Contributor

CMake MR 3128 removes the check that tries the -std=c++1z flag. With that, target_compile_features(... cxx_std_14) should work with nvcc_wrapper.

@ibaned ibaned added the Enhancement Improve existing capability; will potentially require voting label Mar 27, 2019
@jjwilke
Copy link

jjwilke commented Apr 15, 2019

It is going to be a while before this CMake update makes its way into a release number Trilinos requires as the minimum.

I think the only sensible thing to do here is have nvcc_wrapper "downgrade" c++1z to c++14. It would appear we are lucky and that CMake is not actually testing c++1z with a test program that actually requires c++1z.

In general, Kokkos usage should be it is an error to specify -std= directly through CXXFLAGS and it is an error to request "intermediate" standards.

I recommend we just make the EMPIRE fix the current solution until 2 years from now.
@crtrott
@mhoemmen
@ibaned
@ndellingwood

@jjwilke jjwilke self-assigned this Apr 15, 2019
@mhoemmen
Copy link
Contributor

@jjwilke Would this solution (for nvcc_wrapper to "downgrade" from c++1z to c++14) hinder enabling *this capture for device lambdas?

@ibaned
Copy link
Contributor

ibaned commented Apr 15, 2019

@mhoemmen *this capture is only supported officially in C++17, and I think as a compiler extension by NVCC even without specifying C++17, which they don't claim to support all of. I know other Kokkos folks like it, but I don't think its the greatest solution anyways to using lambdas inside class member methods.

@jjwilke
Copy link

jjwilke commented Apr 15, 2019

Dan beat me to it. Are there any versions of nvcc that accept c++1z? Isn't this enabled through the --expt-extended-lambda?

@ibaned
Copy link
Contributor

ibaned commented Apr 15, 2019

from here: https://devblogs.nvidia.com/new-compiler-features-cuda-8/

Note that the *this capture mode is an experimental feature in CUDA 8 and is only supported for extended __device__ lambdas and for lambdas defined within device code, because nvcc does not yet support C++17. It also requires use of the --expt-extended-lambda nvcc command-line flag.

I looks like one doesn't need to mess with --std=c++1z or --std=c++17 to get *this capture in CUDA device lambdas, but this will be non-portable to non-CUDA builds that don't support C++17. Overall, I recommend not using *this capture yet.

@mhoemmen
Copy link
Contributor

@ibaned Thanks for the explanation! We don't need *this capture -- I was just curious if this would affect that.

@jjwilke
Copy link

jjwilke commented Apr 21, 2019

This will break the new CMake which uses target_compile_features. We should decide ASAP on an acceptable solution.

@crtrott
Copy link
Member

crtrott commented May 1, 2019

NVCC doesn't support C++17 yet. There is also no good way to fixing this: lets say we just swap c++1z out for c++14: if cmake actually tries to compile any C++17 feature it will fail. That might be fine though, since it may just set a feature flag to "doesn't work" instead of saying "the compiler doesn't work since the flag is not accepted" - and since down the line cmake fixes the thing, it may be acceptable to do this inside of Trilinos. Another possible long term solution is for CMake not to misidentify nvcc as gcc, when nvcc is used for non-CUDA files.

@jjwilke
Copy link

jjwilke commented May 1, 2019

I meant to update this yesterday. Still fighting the plague. As always, I'm afraid of adding too much implicit behavior. I would much rather crash nvcc_wrapper if it gets anything other than c++11 or c++14 flags. If someone sets CXXFLAGS to c++1z, this could cause really bizarre compiler errors.

There was another sweeping set of changes - now in the cmake-no-gmake branch.
Inside the CMake, I suspect the best thing to do is disallow anything other than c++11 or c++14 for KOKKOS_CXX_COMPILER_ID = NIVIDIA.

We would need to temporarily allow the flag if we are inside CMake. Maybe the best thing to do for now to 1) not silently change people's flags and 2) not cause CMake to crash is literally set an environment variable in CMake and only change the flag if we know that we are inside the CMake feature test (although I'm not sure how exactly we could do this). This isn't super elegant... but gets things working.

@jjwilke
Copy link

jjwilke commented Jun 2, 2019

I believe the new logic in the latest commit addresses this. If you request CMAKE_CXX_STANDARD=14 or KOKKOS_CXX_STANDARD=14 the CMake now recognizes nvcc_wrapper and avoids trying to add cxx_std_14 to the feature set.

For special cases like this where the feature IS supported, but CMake cannot detect that support Kokkos then manually sets the flag to -std=c++14 and makes the flag public. So basically this means that libraries downstream of Kokkos (with nvcc_wrapper, anyway) should not be trying to choose their own C++ standard features and should rely on Kokkos to set these flags.

This is a stop-gap to request C++14 support through standard mechanisms. As soon as minimum CMake gets bumped, we can remove this logic and cxx_std_14 can be used directly with nvcc_wrapper

@jjwilke jjwilke added this to the 3.0 Release milestone Sep 4, 2019
@masterleinad masterleinad added this to To do in Milestone: Release 3.0 via automation Oct 3, 2019
@masterleinad
Copy link
Contributor

I believe this status is good enough for the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
No open projects
Development

No branches or pull requests

10 participants