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

Import test for CUDA project #1285

Closed
wants to merge 5 commits into from
Closed

Import test for CUDA project #1285

wants to merge 5 commits into from

Conversation

@luncliff
Copy link
Contributor

luncliff commented Aug 28, 2019

Summary

This PR adds a new test(fmt-cuda-test) for the library.
It imports fmt in the CUDA(.cu) source file and build it with C++ 14 standard.

find_package(CUDA 9.0)
if(CUDA_FOUND)
  add_subdirectory(cuda-test)
  add_test(NAME cuda-test COMMAND fmt-in-cuda-test)
endif()

Related Issues

Note

The change of each files...

test/CMakeLists.txt

Try to find if the environment has CUDA and add a new test for it(fmt-cuda-test).

find_package(CUDA 9.0)
if(CUDA_FOUND)
  add_test(...) # ... see the change below ...
endif()

The structure followed that of the find-package-test.

test/cuda-test/CMakeLists.txt

The original issue(#1149) was about the case when NVCC's host compiler is MSVC.
However, the case can't be tested always since Windows CI services don't support CUDA build/test.

Instead, the file contains detailed comments. I wish future visitors can read it and solve their issue with it.

test/cuda-test/cpp14.cc

C++ source code for CMake target which requires CUDA compilation

test/cuda-test/cuda-cpp14.cu

CUDA source code for CMake target. This file is a peer of test/cuda-test/cpp14.cc.

<fmt/core.h>

Reviewed #1273.
Following 744302a, the header file checks for NVCC and CUDA source code.

  • __NVCC__: if the compiler is from NVIDIA
  • __CUDACC__: if the source code is CUDA(.cu) file

License Agreement

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

luncliff added 4 commits Aug 28, 2019
Current test is only for Windows(cl.exe).
Need to test more with the other host compilers...

* Activate the test when `find_package(CUDA)` worked
* The test runs with C++14

Detailed comments in 'test/cuda-test'
* checks both `__NVCC__` and `__CUDACC__`

More comments for CMake and CUDA source file.
The header file checks 2 things.

* __NVCC__: if the compiler is from NVIDIA
* __CUDACC__: if the source code is CUDA(.cu) file

Since we can't sure all users prefer latest, Version for
`find_pacakge(CUDA)` is downgraded to 9.0.
This is the minimum version for C++14 in CUDA
@luncliff

This comment has been minimized.

Copy link
Contributor Author

luncliff commented Aug 28, 2019

Tested Environment

  • OS/System
    • Microsoft Windows 10 Pro
    • OS Version/SDK: 10.0.18362 N/A Build 18362
    • System Model: XPS 15 9550
    • System Type: x64-based PC
  • Tools
    • Visual Studio 2017 Community (15.7.13)
    • CMake 3.15.1 (from Chocolatey)
  • CUDA Version: 10.1

Test Run

After its build, the log from the CTest should be like the following.

The test takes some time since it actually invokes NVCC and performs linking to generate the final executable.

PS D:\fmt\build> ctest --output-on-faulure
Test project D:/fmt/build
      Start  1: assert-test
...
      Start 18: cuda-test
18/18 Test #18: cuda-test ........................   Passed   11.53 sec

6% tests passed, 17 tests failed out of 18

Total Test time (real) =  11.99 sec
...
Copy link
Contributor

vitaut left a comment

Thanks for the PR!

// Workaround broken [[deprecated]] in the Intel compiler.
#ifdef __INTEL_COMPILER
# define FMT_DEPRECATED_ALIAS
#else
# define FMT_DEPRECATED_ALIAS FMT_DEPRECATED
#endif

// Workaround broken [[deprecated]] for the NVCC (CUDA with C++14)
#if defined(__NVCC__) || defined(__CUDACC__)

This comment has been minimized.

Copy link
@vitaut

vitaut Aug 29, 2019

Contributor

I suggest merging this into the above #ifdef:

#ifdef __INTEL_COMPILER || defined(__NVCC__) || defined(__CUDACC__)

This comment has been minimized.

Copy link
@luncliff

luncliff Aug 29, 2019

Author Contributor

Sure. can I wrap the Intel compiler's macro in defined? (for consistencty?)

#if defined(__INTEL_COMPILER) || defined(__NVCC__) || defined(__CUDACC__)
#  define FMT_DEPRECATED_ALIAS 
#else 
#  define FMT_DEPRECATED_ALIAS FMT_DEPRECATED 
#endif
#
find_package(CUDA 9.0)
if(CUDA_FOUND)
add_test(cuda-test ${CMAKE_CTEST_COMMAND}

This comment has been minimized.

Copy link
@vitaut

vitaut Aug 29, 2019

Contributor

Can we not invoke cmake recursively and add cuda_add_executable etc. here? We already found the CUDA package here and don't need to find the FMT package since we are within fmt.

This comment has been minimized.

Copy link
@luncliff

luncliff Aug 29, 2019

Author Contributor

What I intended was to show the full example for importing in CUDA project. test/CMakeLists.txt is quite long so I'm afraid it may confuse future readers.
We can consider 2 alternatives.

  • include(fmt-cuda-test.cmake) + add_test: we can consider include to shorten test/CMakeLists.txt
  • add_subdirectory + add_test: I think this is the best
#
# Activate CUDA related tests if we can find CUDA from CMake. This is optional.
# For version selection, see https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#cpp14-language-features
#
find_package(CUDA 9.0)
if(CUDA_FOUND)
  add_subdirectory(cuda-test)
  add_test(NAME cuda-test COMMAND fmt-in-cuda-test)
endif()

This comment has been minimized.

Copy link
@vitaut

vitaut Aug 29, 2019

Contributor

I agree that add_subdirectory is a better option.

#
set_target_properties(fmt-in-cuda-test
PROPERTIES
CXX_STANDARD 14 # Notice this is for C++ code

This comment has been minimized.

Copy link
@vitaut

vitaut Aug 29, 2019

Contributor

This seems redundant since you set cxx_std_14 compiler feature.

This comment has been minimized.

Copy link
@luncliff

luncliff Aug 29, 2019

Author Contributor

I will remove the part (CXX_STANDARD)

get_target_property(cuda_standard
fmt-in-cuda-test CUDA_STANDARD
)
message(STATUS "cuda_standard: ${cuda_standard}")

This comment has been minimized.

Copy link
@vitaut

vitaut Aug 29, 2019

Contributor

Case mismatch?

This comment has been minimized.

Copy link
@luncliff

luncliff Aug 29, 2019

Author Contributor

Oh, that was my mistake :)

message(STATUS "cuda_standard: ${cuda_standard}")

get_target_property(cuda_standard_required
fmt-in-cuda-test CUDA_STANDARD_REQUIRED

This comment has been minimized.

Copy link
@vitaut

vitaut Aug 29, 2019

Contributor

same here

#include <cuda.h>
#include <iostream>

using namespace std;

This comment has been minimized.

Copy link
@vitaut

vitaut Aug 29, 2019

Contributor

nit: Let's just qualify cout.

This comment has been minimized.

Copy link
@luncliff

luncliff Aug 29, 2019

Author Contributor

Got it :)

@vitaut

This comment has been minimized.

Copy link
Contributor

vitaut commented Aug 31, 2019

Merged with minor tweaks in 345ba07. Thanks again.

@vitaut vitaut closed this Aug 31, 2019
@luncliff

This comment has been minimized.

Copy link
Contributor Author

luncliff commented Aug 31, 2019

Appreciate for the acceptance :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.