CMakeLists update for issue 148 #149

ndellingwood · 2018-02-13T19:22:56Z

Address request in issue #148 to check for
CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS=ON when Cuda is enabled.
If it is not on, error out at configuration.

Address request in issue kokkos#148 to check for CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS=ON when Cuda is enabled. If it is not on, error out at configuration.

ndellingwood · 2018-02-13T19:24:23Z

Tested that this 1) errors out at configuration with Cuda8 if CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS is not set on and 2) that it configures properly when the variable is set.

srajama1 · 2018-02-13T23:11:58Z

Is there a problem with always having this ON when CUDA is enabled ? I don't know why we would error out, force the user to set it and reconfigure again. If it is needed can we just set it ON for CUDA.

mhoemmen

Thanks @ndellingwood ! I suggested a couple changes.

mhoemmen · 2018-02-13T23:11:47Z

CMakeLists.txt

@@ -75,6 +75,10 @@ TRIBITS_ADD_OPTION_AND_DEFINE(
    LIST(APPEND DEVICE_LIST "<Cuda,CudaUVMSpace>")
  ENDIF()

+  IF(NOT CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS)


IF(NOT (DEFINED(CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS)) OR (NOT CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS))

etc.

Not sure if CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS is defined by default. (CMake options have three possible states: NOT DEFINED, FALSE, and TRUE.) The above will check for that.

@mhoemmen I didn't realize that, thanks for the catch!

mhoemmen · 2018-02-13T23:14:09Z

CMakeLists.txt

@@ -75,6 +75,10 @@ TRIBITS_ADD_OPTION_AND_DEFINE(
    LIST(APPEND DEVICE_LIST "<Cuda,CudaUVMSpace>")
  ENDIF()

+  IF(NOT CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS)
+    MESSAGE( FATAL_ERROR "Kokkos-Kernels configuration failed: CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS is required be set to ON when compiling with Cuda." )


Incorrect grammar; try this:

The CMake option CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS is either undefined or OFF. Please set CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS:BOOL=ON when building with CUDA.

@mhoemmen made the change, thanks!

@mhoemmen

Fix if check and grammar, suggestions from @mhoemmen

ndellingwood · 2018-02-13T23:55:03Z

Updated PR with suggested changes

ndellingwood · 2018-02-14T00:00:08Z

@bartlettroscoe I added a check in kokkos-kernels for CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS when Cuda is enabled; should this check be moved within Trilinos and, if Cuda and kokkos-kernels are enabled, turn CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS=ON by default so users don't have to reconfigure like @srajama1 mentioned?

bartlettroscoe · 2018-02-14T00:03:12Z

@bartlettroscoe I added a check in kokkos-kernels for CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS when Cuda is enabled; should this check be moved within Trilinos and, if Cuda and kokkos-kernels are enabled, turn CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS=ON by default so users don't have to reconfigure like @srajama1 mentioned?

This file gets processed as part of the Trilinos CMake configure as well so it should be active there as well. In fact, any TriBITS configure of this package will have this check.

mhoemmen · 2018-02-14T02:53:19Z

Thanks @ndellingwood ! :D

mhoemmen

Thanks @ndellingwood ! I have a few questions:

Does a CUDA build with complex OFF actually need this option? If not, then the message isn't quite accurate, though it doesn't hurt.
Does this option work with all supported host compilers for CUDA builds? If not, then we should add a check for the host compiler, just so we don't force users to do something that doesn't work.

ndellingwood · 2018-03-12T19:00:18Z

Thanks @mhoemmen!

I don't believe the Cuda build with complex OFF requires this, I can update so the message and check is more specific.
I have not tested against all supported host compilers, though from the Tribits documentation the possible issues involving use of CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS seem to stem from combination of OS + CMake version + compiler. I'm not sure how to adequately address this, as testing of host compilers from sems modules on a Linux OS may give false confidence that a compiler is compatible, whereas another combo of CMake + OS may fail.

mhoemmen · 2018-03-13T12:30:04Z

@ndellingwood wrote:

I don't believe the Cuda build with complex OFF requires this, I can update so the message and check is more specific.

I'm not too worried about this bit, as long as the build settings work in both complex ON and OFF cases, but it's still helpful to know, so thanks :-) .

I'm not sure how to adequately address this, as testing of host compilers from sems modules on a Linux OS may give false confidence that a compiler is compatible, whereas another combo of CMake + OS may fail.

If we don't test it, it doesn't exist ;-) . I would say, let's discover this rather than trying to guess. If somebody reports on a new platform that it doesn't work, we can change the message then.

Addresses issue kokkos#148. The check for CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS enabled is only necessary when complex+double+Cuda with ETI occurs.

Part of issue kokkos#148

Changes to address issue kokkos#148

ndellingwood · 2018-03-14T19:21:38Z

@crtrott requested the FATAL_ERROR be changed to WARNING in CMakeLists.txt
Updated the PR with improved message and more restricted check (Cuda and complex double enabled) as suggested, and changed replaced with WARNING.
Since the FATAL_ERROR was replaced by a WARNING no one will be forced to use this option which potentially may not work with some combo of host compiler + OS + CMake version.

@srajama1 since CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS will not be enabled by default (discussion and links in issue #176), I think this PR is ready.

ndellingwood · 2018-03-14T20:02:07Z

@srajama1 README updated with comment for usage CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS when building with Trilinos.

mhoemmen

I'm OK with this for now, but please see note. Thanks @ndellingwood !

mhoemmen · 2018-03-14T21:35:22Z

CMakeLists.txt

@@ -75,6 +75,10 @@ TRIBITS_ADD_OPTION_AND_DEFINE(
    LIST(APPEND DEVICE_LIST "<Cuda,CudaUVMSpace>")
  ENDIF()

+  IF( Trilinos_ENABLE_COMPLEX_DOUBLE AND ((NOT DEFINED CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS) OR (NOT CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS)) )


This is fine, though it may be possible to turn OFF KokkosKernels_INST_COMPLEX_DOUBLE even if Trilinos_ENABLE_COMPLEX_DOUBLE is ON.

@mhoemmen Thanks for the catch! I'm guessing most users use Trilinos_ENABLE_COMPLEX_DOUBLE so I think I should leave that and add a check for KokkosKernels_INST_COMPLEX_DOUBLE also, does this sound correct?

@ndellingwood What about stand-alone builds? Does Trilinos_ENABLE_COMPLEX_DOUBLE even exist in that case?

@mhoemmen I'm not sure that kokkos-kernels has standalone CMake support, I think for standalone builds the Makefile system should be used but I may be wrong.

ndellingwood added 2 commits February 13, 2018 11:10

CMakeLists: check for CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS with Cuda

29ac7e7

Address request in issue kokkos#148 to check for CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS=ON when Cuda is enabled. If it is not on, error out at configuration.

CMakeLists: Fix CMake variable syntax

80dee88

ndellingwood requested review from mhoemmen, crtrott and srajama1 February 13, 2018 19:22

ndellingwood mentioned this pull request Feb 13, 2018

Document & check CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS=ON in CUDA build #148

Closed

mhoemmen suggested changes Feb 13, 2018

View reviewed changes

CMakeLists: if check added for kokkos#148 incomplete - updated

086ec01

Fix if check and grammar, suggestions from @mhoemmen

mhoemmen previously approved these changes Feb 14, 2018

View reviewed changes

ndellingwood mentioned this pull request Mar 12, 2018

Require CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS=ON in CUDA, complex ON, ETI ON build #176

Closed

mhoemmen reviewed Mar 12, 2018

View reviewed changes

CMakeLists: Restrict response file check to complex+double+Cuda

73239c9

Addresses issue kokkos#148. The check for CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS enabled is only necessary when complex+double+Cuda with ETI occurs.

ndellingwood dismissed mhoemmen’s stale review via 73239c9 March 14, 2018 17:31

ndellingwood added 2 commits March 14, 2018 11:55

CMakeLists: Fix check for complex double

1a720ff

Part of issue kokkos#148

CMakeLists: Replace FATAL_ERROR with WARNING

2d7326d

Changes to address issue kokkos#148

Update README for CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS usage

3f25e9f

mhoemmen approved these changes Mar 14, 2018

View reviewed changes

srajama1 approved these changes Mar 15, 2018

View reviewed changes

ndellingwood merged commit 28c335c into kokkos:develop Mar 15, 2018

ndellingwood mentioned this pull request Apr 10, 2018

CMake Trilinos CUDA build fails with link error 127 #141

Closed

ndellingwood deleted the issue-148 branch August 30, 2018 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMakeLists update for issue 148 #149

CMakeLists update for issue 148 #149

ndellingwood commented Feb 13, 2018

ndellingwood commented Feb 13, 2018

srajama1 commented Feb 13, 2018

mhoemmen left a comment

mhoemmen Feb 13, 2018

mhoemmen Feb 13, 2018

ndellingwood Feb 13, 2018

mhoemmen Feb 13, 2018

ndellingwood Feb 14, 2018

ndellingwood commented Feb 13, 2018

ndellingwood commented Feb 14, 2018

bartlettroscoe commented Feb 14, 2018

mhoemmen commented Feb 14, 2018

mhoemmen left a comment

ndellingwood commented Mar 12, 2018

mhoemmen commented Mar 13, 2018

ndellingwood commented Mar 14, 2018

ndellingwood commented Mar 14, 2018

mhoemmen left a comment

mhoemmen Mar 14, 2018

ndellingwood Mar 14, 2018

mhoemmen Mar 16, 2018

ndellingwood Mar 16, 2018

CMakeLists update for issue 148 #149

CMakeLists update for issue 148 #149

Conversation

ndellingwood commented Feb 13, 2018

ndellingwood commented Feb 13, 2018

srajama1 commented Feb 13, 2018

mhoemmen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndellingwood commented Feb 13, 2018

ndellingwood commented Feb 14, 2018

bartlettroscoe commented Feb 14, 2018

mhoemmen commented Feb 14, 2018

mhoemmen left a comment

Choose a reason for hiding this comment

ndellingwood commented Mar 12, 2018

mhoemmen commented Mar 13, 2018

ndellingwood commented Mar 14, 2018

ndellingwood commented Mar 14, 2018

mhoemmen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment