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

Enable extended lambdas support by default with CUDA #5580

Merged
merged 2 commits into from
Oct 20, 2022

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Oct 19, 2022

Rational: --expt-extended-lambda is not an experimental flag any more for a long time with NVCC, it is an alias for the --extended-lambda flag and is officially supported. We had agreed for a while that we would use it by default once it would not be considered experimental but somehow never got around to actually implement it. So here it is.

  • Update nvcc_wrapper so it can handle -[-]extended-lambda (w/o the expt- prefix)
  • Change Kokkos_ENABLE_CUDA_LAMBDA option default OFF -> ON
  • Prefer -extended-lambda flag (that change might be problematic if someone was using a pinned version of nvcc_wrapper) edit this controversial bit was dropped from this PR

NOTE: The default value with generated makefiles has not been updated. I am not sure how we would go about that one.

@PhilMiller
Copy link
Contributor

PhilMiller commented Oct 19, 2022

Is there an inherent reason to shift to preferring the newer form -extended-lambda, given that in the short term it may cause friction?

On the other hand, it would be a nice nudge for people to update a pinned/vendored/snapshot nvcc_wrapper if they have one

@PhilMiller
Copy link
Contributor

I'm +1 on accepting the non--expt flag and enabling the feature by default

@JBludau
Copy link
Contributor

JBludau commented Oct 19, 2022

I think this is more then reasonable considering it will be in a major release, even if some people have a version pinned

@dalg24
Copy link
Member Author

dalg24 commented Oct 19, 2022

Is there an inherent reason to shift to preferring the newer form -extended-lambda, given that in the short term it may cause friction?

No reason to prefer it. I realize that might be an issue as I was opening the PR.

On the other hand, it would be a nice nudge for people to update a pinned/vendored/snapshot nvcc_wrapper if they have one

Let me know if you want me to drop the commit that makes us add the non prefixed flag

@PhilMiller
Copy link
Contributor

Let's get the entirely non-controversial bits merged quickly, and then we can figure out the other bit

@dalg24
Copy link
Member Author

dalg24 commented Oct 20, 2022

I dropped the commit instead of reverting. Pasting it below for the record.

commit 1a9ded47da2bb55158c42475e1d54ec4379193b9
Author: Damien L-G <dalg24@gmail.com>
Date:   Wed Oct 19 08:20:30 2022 -0400

    Prefer -expt-extended-lambda -> -extended-lambda with NVCC

diff --git a/Makefile.kokkos b/Makefile.kokkos
index 5cabbdc53..03441735d 100644
--- a/Makefile.kokkos
+++ b/Makefile.kokkos
@@ -667,7 +667,7 @@ ifeq ($(KOKKOS_INTERNAL_USE_CUDA), 1)
   ifeq ($(KOKKOS_INTERNAL_CUDA_USE_LAMBDA), 1)
     ifeq ($(KOKKOS_INTERNAL_COMPILER_NVCC), 1)
       tmp := $(call kokkos_append_header,"$H""define KOKKOS_ENABLE_CUDA_LAMBDA")
-      KOKKOS_CXXFLAGS += -expt-extended-lambda
+      KOKKOS_CXXFLAGS += -extended-lambda
     endif

     ifeq ($(KOKKOS_INTERNAL_COMPILER_CLANG), 1)
diff --git a/cmake/kokkos_arch.cmake b/cmake/kokkos_arch.cmake
index f8aa4484f..5374f06a2 100644
--- a/cmake/kokkos_arch.cmake
+++ b/cmake/kokkos_arch.cmake
@@ -168,7 +168,7 @@ GLOBAL_SET(KOKKOS_CUDA_OPTIONS)
 # Construct the Makefile options
 IF (KOKKOS_ENABLE_CUDA_LAMBDA)
   IF(KOKKOS_CXX_COMPILER_ID STREQUAL NVIDIA)
-    GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS "-expt-extended-lambda")
+    GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS "-extended-lambda")
     GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS "-Wext-lambda-captures-this")
   ENDIF()
 ENDIF()

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'm fine with discussing the Makefile support elsewhere.

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

4 participants