Panzer: including `Panzer_FieldPattern.hpp` will also include files like `KokkosExp_View_Fad.hpp` from `Sacado`

[romintomasetti]

Enhancement

When someone includes Panzer_FieldPattern.hpp, it also implicitly includes KokkosExp_View_Fad.hpp (and others) because of the include Phalanx_KokkosDeviceTypes.hpp.

I think 2 relevant comments can be made at this point:

1. Looking at PHX::Device, it seems it can safely be replaced by Kokkos::DefaultExecutionSpace. This would alleviate the need for including Phalanx_KokkosDeviceTypes.hpp in Panzer_FiledPattern.hpp, and solves the problem.
2. It seems that there is not a good separation of purposes in Phalanx_KokkosDeviceTypes.hpp. It defines the PHX::Device and that's OK regarding the file name. But it also defines traits for views with Sacado. These traits should be moved elsewhere.

I think this is a big issue for compilation time. We've seen our code being very resource intensive to compile and these kind of "wrong include files" may explain why.

I'd be glad to make a PR at least to remove PHX::Device and default to Kokkos::DefaultExecutionSpace if you agree, because I really feel it's obsolete.

Thanks!

[cgcgcg]

@trilinos/panzer

[rppawlo]

We have use cases where we allow users to pick the PHX::Device as a cmake configure option. For example, if Kokkos is enabled for both OpenMP and CUDA, phalanx will be restricted to running in one of those device spaces. For convenience, users can pick this at compile time and use the other execution space for other work (usually in code coupling to another app). Early on, we chose not to expose the device space as a template parameter in MDFields for backwards compatiblity. The cmake flag was the only way to control the device to run on with multipel execution spaces enabled. Th restriction in mdfields has been recently removed so picking the device space at compile time is less of an issue, but forces users to explicitly set spaces when using phalanx objects. So I think we could always default to the kokkos default, but we would lose some flexibility (or force app codes to be more verbose in their use of Phalanx objects.

I agree that splitting out the phalanx defines for sacado into a separate file is worth doing, but it will take some exploration and might not be possible. Sacado overrides some definitions in kokkos and this requires the sacado file be included before the kokkos file. We've had a lot of issues with this order of includes. Removing kokkos here may break a ton of downstream code in applications via a non-backwards compatible change. I'd like to explore this more with sandia apps before making such a change.

If you do this locally in your clone I would be interested to know if the compile times are really impacted. I had a similar discussion with the kokkos team. The Kokkoc_Core.hpp file pulls in the entire libaray of std math functions on device even if you don't need it. They argued that the parsing did not significantly impact run times. I'm curious to see if the sacado includes really do impact compile times.

I will be able to look into this in another week or two if that timeline is ok.

[romintomasetti]

1. If I understand it correctly, a user might want to set the Phalanx device type using PHX_KOKKOS_DEVICE_TYPE to be different from what Kokkos::DefaultExecutionSpace is. I agree that this is OK. However, I think the CMake/C++ logic could be reduced to something like:

   // If the user defined PHX_KOKKOS_DEVICE_TYPE to e.g. "SERIAL", then CMake adds the compile definition `PHX_KOKKOS_DEVICE_TYPE=Kokkos::Serial`.
   // Otherwise, default to Kokkos default.
   #if defined(PHX_KOKKOS_DEVICE_TYPE)
     using PHX::Device = PHX_KOKKOS_DEVICE_TYPE;
   #else
     using PHX::Device = Kokkos::DefaultExecutionSpace;
   #endif

2. I tend to agree with the Kokkos team for compilation time only partially.

   Maybe in Kokkos the impact is small because there are only a few "nested" includes, but for libraries like Panzer that depend on many others, it might have an impact (also thinking about "slow" compilers like nvcc). Not to mention user apps that depend on Trilinos but also on other tools. Also, as the number of tests/executables grow, parsing useless-but-still-included files is really a waste of resources.

   Also, I tend to disagree with the Kokkos policy that one should include everything through Kokkos_Core.hpp. This goes counter-current against the good practice of "include what you use" (https://google.github.io/styleguide/cppguide.html#Include_What_You_Use).

[romintomasetti]

1. If I understand it correctly, a user might want to set the Phalanx device type using PHX_KOKKOS_DEVICE_TYPE to be different from what Kokkos::DefaultExecutionSpace is. I agree that this is OK. However, I think the CMake/C++ logic could be reduced to something like:

   // If the user defined PHX_KOKKOS_DEVICE_TYPE to e.g. "SERIAL", then CMake adds the compile definition `PHX_KOKKOS_DEVICE_TYPE=Kokkos::Serial`.
   // Otherwise, default to Kokkos default.
   #if defined(PHX_KOKKOS_DEVICE_TYPE)
     using PHX::Device = PHX_KOKKOS_DEVICE_TYPE;
   #else
     using PHX::Device = Kokkos::DefaultExecutionSpace;
   #endif

   Do you agree?

2. I tend to agree with the Kokkos team for compilation time only partially.

   Maybe in Kokkos the impact is small because there are only a few "nested" includes, but for libraries like Panzer that depend on many others, it might have an impact (also thinking about "slow" compilers like nvcc). Not to mention user apps that depend on Trilinos but also on other tools. Also, as the number of tests/executables grow, parsing useless-but-still-included files is really a waste of resources.

   Also, I tend to disagree with the Kokkos policy that one should include everything through Kokkos_Core.hpp. This goes counter-current against the good practice of "include what you use" (https://google.github.io/styleguide/cppguide.html#Include_What_You_Use).

[github-actions[bot]]

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

[github-actions[bot]]

This issue was closed due to inactivity for 395 days.

[romintomasetti]

@cgcgcg @rppawlo Could you reopen this one ? I think it's not over yet :smile: Thanks !