sandialabs / Albany

Sandia National Laboratories' Albany multiphysics code
Other
282 stars 89 forks source link

Undefined references when using new Cuda-UVM flags #1025

Open mcarlson801 opened 10 months ago

mcarlson801 commented 10 months ago

It looks like we're now getting a bunch of undefined references on Weaver now that I've turned on the new Cuda UVM flags. The errors can be seen here: https://sems-cdash-son.sandia.gov/cdash/viewBuildError.php?buildid=56941

An example:

tmpxft_001e528f_00000000-6_Albany_Utils.cudafe1.cpp:(.text+0x164c): undefined reference to `Tpetra::MultiVector<double, int, long long, Tpetra::KokkosCompat::KokkosDeviceWrapperNode<Kokkos::Cuda, Kokkos::CudaSpace> >::putScalar(double const&)'

It looks like KokkosDeviceWrapper is looking for functions with CudaSpace template arguments instead of CudaUVMSpace.

I'm not sure exactly when this showed up since I forgot to update the weaver scripts when I updated them in the repo so we only started nightly testing of this today. It worked when I tested https://github.com/trilinos/Trilinos/pull/12626 so this showed up sometime between now and then.

mcarlson801 commented 10 months ago

Tagging @csiefer2 and @mperego for visibility

I reverted the weaver dashboard to the old flag for the time being so we don't lose coverage.

mperego commented 9 months ago

@cgcgcg FYI, we have seen these errors when enabling UVM with the new flags. Hopefully they will be caught by the new PR https://github.com/trilinos/Trilinos/issues/12767.

cgcgcg commented 9 months ago

Are you sure Albany is correctly set up to use the Tpetra shared spaces flag? I see this https://github.com/sandialabs/Albany/blob/93c7e769073c800845c58bb504961209f1714f83/src/Albany_TpetraTypes.hpp#L52-L62 and this https://github.com/sandialabs/Albany/blob/93c7e769073c800845c58bb504961209f1714f83/src/Albany_KokkosTypes.hpp#L29 but nothing in Phalanx is using the Tpetra shared space stuff.

mperego commented 8 months ago

@rppawlo Christian helped me understand a bit better this issue. Our goal is to use Cuda UVM without using the deprecated -D Kokkos_ENABLE_CUDA:BOOL=ONoption. Tpetra and Kokkos Kernels allow to use CUDA UVM by setting

-D KokkosKernels_INST_MEMSPACE_CUDAUVMSPACE=ON
-D Tpetra_ALLOCATE_IN_SHARED_SPACE=ON

However, Phalanx does not allow us to do so. At the moment, in Albany we use Phalanx::Device everywhere, and we end up having issues because our Tpetra vector and maps and matrices use Phalanx::Device, which doesn't use Cuda UVM, whereas Tpetra is expecting us to use Cuda UVM. The most natural options for us would be for Phalanx to use Cuda UVM when Tpetra is, or, alternatively, to allow Albany to set a DeviceType for Phalanx (I know that you have worked on a branch that is supposed to do so, and it's probably the best solution for Trilinos). Let us know what you think. This is not urgent, but we would like to have this addressed at some point and we can probably put some resources into it.

bartgol commented 8 months ago

I don't think we should make Phalanx look at what Tpetra uses, b/c PHX has no dependence on Tpetra.

Probably the cleanest solution would be to add a -DPhalanx_ENABLE_SHARED_SPACE=ON cmake option for phalanx, cmakedefine it in Phalanx_config.hpp.in, and then in Phalanx_KokkosDeviceTypes.hpp do

  using exec_space = PHX::Device::execution_space;
  using ExecSpace  = PHX::Device::execution_space;

#ifdef PHALANX_ENABLE_SHARED_SPACE
  using MemSpace   = Kokkos::SharedSpace;
  using mem_space  = Kokkos::SharedSpace;
#else
  using MemSpace   = PHX::Device::memory_space;
  using mem_space  = PHX::Device::memory_space;
#endif

It is up to the downstream app then to ensure that Tpetra and PHX use compatible spaces. If they so desire. There is no a-priori need to have PHX and Tpetra use the same mem space (though it is probably convenient).

rppawlo commented 8 months ago

@bartgol 's comment is probably the fastest way to accomplish uvm support. There would have to be some changes to the memory allocators as well (make sure phalanx uses MemSpace consistently and still allow users to override when needed). We do not want to introduce a dependency between phalanx and tpetra just to get a default type. There is a branch that makes the PHX::Device a true Kokkos::Device (my preferred long term solution), but that causes a mess of backwards incompatible changes. It severely impacts the downstream apps. The branch is in my personal fork of trilinos: rppawlo/phalanx-device-object-refactor if you want to take a look. I do have some refactoring tools as well that get you the 90% solution. We should have a quick meeting about this.

bartgol commented 8 months ago

I think that setting the Device would not solve the problem. The issue is the memory space that PHX uses. Without using the deprecated Kokkos_ENABLE_CUDA_UVM, the mem space of the Cuda device will always be CudaSpace. It is up to the kokkos customer (PHX in this case) to use Kokkos::SharedSpace as a memory space, rather than DeviceType::memory_space (and this can be done on a per-view basis as well!).

mperego commented 8 months ago

Sounds good. I didn't think of the fact that Phalanx doesn't depend on Tpetra

@bartgol The "true" Kokkos::Device, not the current PHX::Device, let you set both the execution space and the memory space, e.g. Kokkos::Device<Kokkos::Cuda,Kokkos::CudaUVMSpace>. But I might be misunderstanding what you are saying.

@rppawlo, sounds good, let's have a quick meeting when you have time.

bartgol commented 8 months ago

Ah, you're right. I confused Kokkos::Device with the exec space. Makes sense then.