[PATCH] D19434: [test-suite] Added CMake files for external CUDA tests.

[Artem Belevich via llvm-commits]

tra added inline comments.

================
Comment at: External/CUDA/CMakeLists.txt:83-88
@@ +82,8 @@
+function(create_thrust_tests VariantSuffix)
+  list(APPEND CPPFLAGS
+    -DTHRUST_HOST_SYSTEM=THRUST_HOST_SYSTEM_CPP
+    -DTHRUST_DEVICE_SYSTEM=THRUST_DEVICE_SYSTEM_CUDA)
+  list(APPEND CPPFLAGS
+    -I${CudaPath}/include
+    -I${THRUST_PATH} -I${THRUST_PATH}/testing)
+
----------------
MatzeB wrote:
> It's unusual to do stuff like setting CPPFLAGS in a function rather than toplevel. At least in the sense that it is inconsistent with the style in the rest of the test-suite.
Part of the problem is that cmake wants to have one configuration for *everything* while I want to build multiple variants of the same test without creating separate build directory for each, configure things there, etc. because I'd need *a lot* of them. My current test set involves 12 build variants. Once CUDA-8 is out, it 's likely to become 24.

Also, within each build variant I need to build more than one application, each of them potentially requiring its own set of compiler flags. Hence I create thrust app inside a function which gives each application its own set of flags within build variant. 

Granted, I may be forcing CMake to do something it was not intended for. I'm new to CMake, so if you have suggestions how to do it better, I'd appreciate your input.

I guess I could move thrust into its own variant iteration loop and set compiler flags that are identical for all variants outside of the loop. It would not do much to change the fact that I'll have large number of targets that each needs its own set of compiler options and libraries to link with.

================
Comment at: External/CUDA/CMakeLists.txt:114
@@ +113,3 @@
+    set(Source ${ThrustTestFramework} ${ThrustAllTestSources})
+    if(DEFINED LARGE_PROBLEM_SIZE)
+      llvm_test_run(--verbose --sizes=large)
----------------
MatzeB wrote:
> `if(LARGE_PROBLEM_SIZE)` is probably enough.
Fixed.

================
Comment at: External/CUDA/CMakeLists.txt:132-159
@@ +131,30 @@
+# Sets VARIANT_CUDA_TESTS targets in parent's scope.
+function(create_cuda_test_variant CudaPath Std GccPath)
+  get_version(CudaVersion ${CudaPath})
+  set(VariantSuffix "cuda-${CudaVersion}")
+  list(APPEND CPPFLAGS --cuda-path=${CudaPath})
+  list(APPEND VariantLibs cudart-${CudaVersion})
+
+  if(Std)
+    set(VariantSuffix "${VariantSuffix}-${Std}")
+    list(APPEND CPPFLAGS -std=${Std})
+    list(APPEND LDFLAGS -std=${Std})
+  endif()
+
+  if(GccPath)
+    get_version(GccVersion ${GccPath})
+    set(VariantSuffix "${VariantSuffix}-libstdc++-${GccVersion}")
+
+    # Tell clang to use libstdc++ and where to find it.
+    list(APPEND CPPFLAGS -gcc-toolchain ${GccPath}/usr/local -stdlib=libstdc++)
+    # Add libstdc++ as link dependency.
+    list(APPEND VariantLibs libstdcxx-${GccVersion})
+  else()
+    # Tell clang to use libc++
+    # We also need to add compiler's include path for cxxabi.h
+    get_filename_component(_compiler_path ${CMAKE_CXX_COMPILER} DIRECTORY)
+    list(APPEND CPPFLAGS -stdlib=libc++ -I${_compiler_path}/../include)
+    list(APPEND LDFLAGS -stdlib=libc++)
+    list(APPEND VariantLibs libcxx)
+  endif()
+
----------------
MatzeB wrote:
> This looks like magic to discover the host system. Maybe this would better fit in some global configuration variables combines with cmake/cache files for typical configurations? Esp. the hardcoding of the libstdc++ paths looks out of place here...
Not quite. I need to test with different libc++ variants provided in the externals directory. In order to do that I need to provide explicit path to libstdc++ shared library so executable is linked with appropriate -Wl,rpath and picks correct shared library when it's executed. Otherwise I end up loading whatever libstdc++ is installed on host or failing to run because it's not found.

Again, it's probably something I can move out of per-variant loop, but at the moment I can't think of a way to do just let CMake magically pick it only for specific subset of targets I'm creating.

================
Comment at: External/CUDA/CMakeLists.txt:175-189
@@ +174,17 @@
+macro(create_cuda_tests)
+  message(STATUS "Checking CUDA prerequisites in ${TEST_SUITE_CUDA_ROOT}")
+  file(GLOB CudaVersions ${TEST_SUITE_CUDA_ROOT}/cuda-*)
+  foreach(CudaDir IN LISTS CudaVersions)
+    get_version(CudaVersion ${CudaDir})
+    message(STATUS "Found CUDA ${CudaVersion} ${CudaDir}")
+    list(APPEND CUDA_PATHS ${CudaDir})
+    add_library(cudart-${CudaVersion} SHARED IMPORTED)
+    set_property(TARGET cudart-${CudaVersion} PROPERTY IMPORTED_LOCATION
+      ${CudaDir}/lib64/libcudart.so)
+  endforeach(CudaDir)
+
+  if(NOT CUDA_PATHS)
+    message(SEND_ERROR
+      "There are no CUDA installations in ${TEST_SUITE_CUDA_ROOT}")
+    return()
+  endif()
----------------
MatzeB wrote:
> ditto.
Similar story here. Per CUDA version installed I need to link with specific cuda runtime library and provide specific include paths. Can't set it globally for everyone. I can move it out of the per-variant loop and do it once, though.



================
Comment at: External/CUDA/CMakeLists.txt:202-222
@@ +201,23 @@
+  file(GLOB GccVersions ${TEST_SUITE_CUDA_ROOT}/gcc-*)
+  foreach(GccDir IN LISTS GccVersions)
+    get_version(GccVersion ${GccDir})
+    message(STATUS "Found GCC ${GccVersion}")
+    list(APPEND GCC_PATHS ${GccDir})
+    add_library(libstdcxx-${GccVersion} SHARED IMPORTED)
+    set_property(TARGET libstdcxx-${GccVersion} PROPERTY IMPORTED_LOCATION
+      ${GccDir}/usr/local/lib64/libstdc++.so)
+  endforeach(GccDir)
+
+  # Find location of libc++
+  execute_process(
+    COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=libc++.so
+    OUTPUT_VARIABLE _path_to_libcxx
+    OUTPUT_STRIP_TRAILING_WHITESPACE)
+
+  if (EXISTS ${_path_to_libcxx})
+    add_library(libcxx SHARED IMPORTED)
+    set_property(TARGET libcxx PROPERTY IMPORTED_LOCATION ${_path_to_libcxx})
+  else()
+    message(ERROR "Can't find libcxx location.")
+    return()
+  endif()
----------------
MatzeB wrote:
> ditto.
I need to find *specific* libc++/libstdc++ library to link with. CMake's standard "find me something" facilities are not flexible enough for what I need here.

================
Comment at: External/CUDA/README:4-6
@@ +3,5 @@
+
+Cuda tests are enabled if cmake is invoked with
+-DTEST_SUITE_EXTERNALS_DIR=<externals path> and specified externals
+directory contains at least one CUDA installation.
+
----------------
MatzeB wrote:
> Note that as a convenience you can even do without this flag: If you checkout your externals in `test-suite/test-suite-externals` it will get picked up automatically at cmake time.
Good to know. I'll add this info.


http://reviews.llvm.org/D19434