Remove Kokkos_ENABLE_CUDA_LDG_INTRINSIC option #5623

masterleinad · 2022-11-07T19:18:52Z

Related to #5621. Since #2156, KOKKOS_ENABLE_CUDA_LDG_INTRINSIC is always enabled and, hence, CudaTextureFetch is unused.
This pull request proposes to eventually remove the option and CudaTextureFetch since it hasn't been used in more than 3 years.

dalg24

Please discuss why it is OK to drop the KOKKOS_ENABLE_CUDA_LDG_INTRINSIC macro

dalg24 · 2022-11-07T21:05:59Z

Makefile.kokkos

@@ -77,6 +77,7 @@ KOKKOS_INTERNAL_DISABLE_COMPLEX_ALIGN := $(call kokkos_has_string,$(KOKKOS_OPTIO
 KOKKOS_INTERNAL_DISABLE_DUALVIEW_MODIFY_CHECK := $(call kokkos_has_string,$(KOKKOS_OPTIONS),disable_dualview_modify_check)
 KOKKOS_INTERNAL_ENABLE_PROFILING_LOAD_PRINT := $(call kokkos_has_string,$(KOKKOS_OPTIONS),enable_profile_load_print)
 KOKKOS_INTERNAL_ENABLE_LARGE_MEM_TESTS := $(call kokkos_has_string,$(KOKKOS_OPTIONS),enable_large_mem_tests)
+# deprecated


I see that they are two other occurrences of us placing the # deprecated comment on the line before but I am wondering if there is any good reason not to place it at the end of the line that defines the variable?

I find it better visible to put it on a separate line (and that's also what #5608 does).

cmake/KokkosCore_config.h.in

core/src/Cuda/Kokkos_Cuda_View.hpp

dalg24 · 2022-11-07T23:12:49Z

BUILD.md

    * Whether to use CUDA LDG intrinsics
-    * BOOL Default: OFF
+    * BOOL Default: ON


Did we intentionally leave

kokkos/cmake/kokkos_enable_options.cmake

Line 34 in 7d2b292

KOKKOS_ENABLE_OPTION(CUDA_LDG_INTRINSIC OFF "Whether to use CUDA LDG intrinsics")

Yes, I was assuming that people if at all set Kokkos_ENABLE_CUDA_LDG_INTRINSIC=ON and not Kokkos_ENABLE_CUDA_LDG_INTRINSIC=OFF explicitly. Hence, I would only warn/error out of people set it to ON (and that's why the "real" default should be OFF). Any better suggestions?

The option needs to only be defined if deprecated code 4 is ON. I am torn whether I would bother changing the documentation of what the default might be. I am leaning towards no.

masterleinad · 2022-11-08T14:15:18Z

Please discuss why it is OK to drop the KOKKOS_ENABLE_CUDA_LDG_INTRINSIC macro

I reverted it for it always to be defined (but not used).

dalg24 · 2022-11-08T15:57:27Z

Please discuss why it is OK to drop the KOKKOS_ENABLE_CUDA_LDG_INTRINSIC macro

I reverted it for it always to be defined (but not used).

Did you consider defining it only if deprecated code 4 is ON? How likely is it that this macro is being used downstream? Did you check in Trilinos? etc.

masterleinad · 2022-11-08T16:27:44Z

Did you consider defining it only if deprecated code 4 is ON? How likely is it that this macro is being used downstream? Did you check in Trilinos? etc.

There really is no good reason why the macro would be used downstream, though; it should be purely internal. Trilinos doesn't use it (except for the bundled Kokkos of course). My initial instinct was to remove it because of that but that might be strong. There is also KOKKOS_OPT_RANGE_AGGRESSIVE_VECTORIZATION which we should have removed by now. In view of that, it might be best to remove it conditionally.

Anyway, the more important question is, of course, if we are OK with removing the option (and with it the CudaTextureFetch branch) in the first place.

…_config.h.in

dalg24 · 2022-11-08T20:11:03Z

Anyway, the more important question is, of course, if we are OK with removing the option (and with it the CudaTextureFetch branch) in the first place.

I don't expect anyone is going to argue against removing an option for which we effectively ignore the value set by the user nor keep a dead branch in our code.

There really is no good reason why the macro would be used downstream, though; it should be purely internal. Trilinos doesn't use it (except for the bundled Kokkos of course). My initial instinct was to remove it because of that but that might be strong.

I tend to agree and don't expect one can do anything useful in user code based on whether that macro was defined or not. I would err on the side of caution and preserve the macro when deprecated code 4 is ON.

dalg24 · 2022-11-08T20:12:43Z

cmake/KokkosCore_config.h.in

+#ifdef KOKKOS_ENABLE_DEPRECATED_CODE_4
+#define KOKKOS_ENABLE_CUDA_LDG_INTRINSIC  // deprecated
+#endif
+


Belongs to <Kokkos_Macros.hpp>

Also we only want to define it if CUDA is enabled.

dalg24 · 2022-11-08T20:23:32Z

BUILD.md

    * Whether to use CUDA LDG intrinsics
-    * BOOL Default: OFF
+    * BOOL Default: ON


The option needs to only be defined if deprecated code 4 is ON. I am torn whether I would bother changing the documentation of what the default might be. I am leaning towards no.

dalg24 · 2022-11-08T20:25:33Z

cmake/kokkos_enable_options.cmake

@@ -141,3 +141,11 @@ ENDIF()
 IF (KOKKOS_ENABLE_CUDA_RELOCATABLE_DEVICE_CODE AND BUILD_SHARED_LIBS)
  MESSAGE(FATAL_ERROR "Relocatable device code requires static libraries.")
 ENDIF()
+
+IF(KOKKOS_ENABLE_CUDA_LDG_INTRINSIC)


You would do KOKKOS_ -> Kokkos_ here if the option is conditionally declared.

dalg24 · 2022-11-08T20:26:27Z

cmake/kokkos_enable_options.cmake

+  IF(KOKKOS_ENABLE_DEPRECATED_CODE_4)
+    MESSAGE(DEPRECATION "Setting Kokkos_ENABLE_CUDA_LDG_INTRINSIC is deprecated. LDG intrinsics are always enabled.")
+  ELSE()
+    MESSAGE(FATAL_ERROR "Kokkos_ENABLE_DEPRECATED_CODE_4 must be set to use Kokkos_ENABLE_CUDA_LDG_INTRINSIC")


Suggested change

MESSAGE(FATAL_ERROR "Kokkos_ENABLE_DEPRECATED_CODE_4 must be set to use Kokkos_ENABLE_CUDA_LDG_INTRINSIC")

MESSAGE(FATAL_ERROR "Kokkos_ENABLE_CUDA_LDG_INTRINSIC has been removed. LDG intrinsics are always enabled.")

dalg24

Please confirm that you meant to define the option regardless of whether deprecated code 4 is enabled or not

kokkos/cmake/kokkos_enable_options.cmake

Line 34 in 21d3e48

    
           KOKKOS_ENABLE_OPTION(CUDA_LDG_INTRINSIC   OFF "Whether to use CUDA LDG intrinsics")

dalg24

Will merge as is and we will do a follow up if necessary

Remove Kokkos_ENABLE_CUDA_LDG_INTRINSIC

7d2b292

masterleinad force-pushed the remove_cuda_ldg_option branch from e1b27d2 to 7d2b292 Compare November 7, 2022 20:31

dalg24 reviewed Nov 8, 2022

View reviewed changes

Define KOKKOS_ENABLE_CUDA_LDG_INTRINSIC again

912e821

Conditionally remove KOKKOS_ENABLE_CUDA_LDG_INTRINSIC from KokkosCore…

5559be3

…_config.h.in

dalg24 reviewed Nov 8, 2022

View reviewed changes

masterleinad added 3 commits November 8, 2022 16:28

Move KOKKOS_ENABLE_CUDA_LDG_INTRINSIC to Kokkos_Macros.hpp

a2749b6

Improve error message

0b85736

Update documentation

21d3e48

ajpowelsnl self-assigned this Nov 8, 2022

ajpowelsnl approved these changes Nov 8, 2022

View reviewed changes

cwpearson mentioned this pull request Nov 9, 2022

Remove two-argument constructor for CudaLDGFetch #5621

Merged

dalg24 reviewed Nov 29, 2022

View reviewed changes

dalg24 approved these changes Nov 29, 2022

View reviewed changes

dalg24 merged commit d802fd0 into kokkos:develop Nov 29, 2022

masterleinad mentioned this pull request Nov 30, 2022

CHANGELOG: 4.0 #5439

Closed

masterleinad mentioned this pull request May 11, 2023

Cuda: Remove unused attach_texture_object #6129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Kokkos_ENABLE_CUDA_LDG_INTRINSIC option #5623

Remove Kokkos_ENABLE_CUDA_LDG_INTRINSIC option #5623

masterleinad commented Nov 7, 2022

dalg24 left a comment

dalg24 Nov 7, 2022

masterleinad Nov 8, 2022

dalg24 Nov 7, 2022

masterleinad Nov 8, 2022

dalg24 Nov 8, 2022

masterleinad commented Nov 8, 2022

dalg24 commented Nov 8, 2022

masterleinad commented Nov 8, 2022

dalg24 commented Nov 8, 2022

dalg24 Nov 8, 2022

dalg24 Nov 8, 2022

dalg24 Nov 8, 2022

dalg24 Nov 8, 2022

dalg24 Nov 8, 2022

dalg24 left a comment

dalg24 left a comment

	MESSAGE(FATAL_ERROR "Kokkos_ENABLE_DEPRECATED_CODE_4 must be set to use Kokkos_ENABLE_CUDA_LDG_INTRINSIC")
	MESSAGE(FATAL_ERROR "Kokkos_ENABLE_CUDA_LDG_INTRINSIC has been removed. LDG intrinsics are always enabled.")

Remove Kokkos_ENABLE_CUDA_LDG_INTRINSIC option #5623

Remove Kokkos_ENABLE_CUDA_LDG_INTRINSIC option #5623

Conversation

masterleinad commented Nov 7, 2022

dalg24 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

masterleinad commented Nov 8, 2022

dalg24 commented Nov 8, 2022

masterleinad commented Nov 8, 2022

dalg24 commented Nov 8, 2022

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

dalg24 left a comment

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment