Tpetra: track MultiVector DualView refactoring

[brian-kelley]

Work Tracking Issue

@trilinos/tpetra

Branch for working on MultiVector DualView management refactor: TpetraDualViewRefactor.

Updated 2-3-21:
I replaced the new functions we just added (e.g. getLocalViewDeviceConst, getLocalViewHostNonConst) with permament tagged versions:

//New interface:
#include "Tpetra_Access.hpp"
//...
MultiVector::getLocalView<Device>(Tpetra::Access::ReadOnly());
MultiVector::getLocalView<Device>(Tpetra::Access::ReadWrite());
MultiVector::getLocalView<Device>(Tpetra::Access::WriteOnly());

Non-templated versions like getLocalViewHost(tag) too.
All getLocalView calls in MultiVector and core/test/MultiVector have been replaced with this new interface, but this caused new failures in tests.

MultiVector

• TpetraCore_MultiVector_UnitTests_MPI_4
• TpetraCore_Transform_MPI_4
• TpetraCore_Bug7758_MPI_4
• TpetraCore_Bug7745_MPI_4
• TpetraCore_MultiVector_get2dView_MPI_1
• TpetraCore_MultiVector_TwoArgRandomize_MPI_1
• TpetraCore_Issue364_MPI_1
• TpetraCore_MultiVector_MicroBenchmark_MPI_1 //works on 4 ranks, doesn't work on 1
• TpetraCore_deep_copy_SerialDenseMatrix_to_MultiVector_MPI_1
• TpetraCore_deep_copy_MultiVector_to_SerialDenseMatrix_MPI_1
• TpetraCore_MV_reduce_strided_MPI_4 //Brian
• TpetraCore_aliased_deep_copy_MPI_4

FEMultiVector

• TpetraCore_FEMultiVector_UnitTests_MPI_4. // Can't fix until CrsMatrix is fixed
• TpetraCore_FEMultiVector_Fix3101_MPI_4

BlockMultiVector

• BlockMultiVector_MPI_4
• BlockMultiVector2_MPI_4
• BlockMap_MPI_4.

Import/Export

• Issue3968

[kyungjoo-kim]

@brian-kelley would you put your branch name here so that I can fork from yours ?

[kddevin]

Branch for working on MultiVector DualView management refactor: TpetraDualViewRefactor.

[kyungjoo-kim]

Oh.... the branch is from Trilinos repo. I looked for it from brian's forked repo. Now I got it.

[kyungjoo-kim]

in order not to duplicate effort,

• item 14,17, 20, 23, 28, 29, 35 are done.

Item 30.

This test a workflow 1) a user creates a device view, 2) tpetra wraps the device view in the dual view structure, 3) check data consistency when it modifies the user created device local view.

This workflow is the thing that we do not want users to do. When a user creates a device view and hand it to Tpetra, it breaks the Tpetra dual view logic as the user hold the local object (reference count is unmatched when Tpetra is created). If we want to give this option to users, we need to allocate the dual view and make deep copy and users local device view lose the link to Tpetra. Soft copy of device(or host) view to dual view is not working if we want to keep the transactional behavior on the tpetra dual view context.

Tpetra team reaches work-around for the item 30.

Leave the interface for wrapping a device view interface (soft copy to tpetra dual view device view or host view) which allows reference counting does not match when tpetra object is constructed. However, the testing scenario described in item 30 is the anti-pattern that we want to prevent it from users. So, we just do not test the part of the anti-pattern test.

[kyungjoo-kim]

Issue 364 is done.

[csiefer2]

OK. So I removed every reference to getLocalViewHost() and getLocalViewDevice() in Tpetra and the tests and replaced them with the appropriate const and non-const version.

Complications:

1. Tpetra::MultiVector::description() should not do any sync'ing. To do this I added an "unsafe" function with the old sync-less behavior. There are other ways to do this that might be better We should discuss this tomorrow.

[kddevin]

List of things to do before merging:

• switch to use access tags (ReadOnly, WriteOnly, ReadWrite) // Karen
• remove sync/modify from all MV usage //Kyungjoo
• make sure MV tests work without UVM // Brian: they do as of 2-19-21, so now make sure they don't break again
• make sure all tests work with UVM (a la PR testing)
• disable old interface if and only if CUDA is ON and UVM is OFF. //Timothy

Eventually, but not necessarily for PR merge

• formally deprecate old functions (might do when CrsGraph/Matrix deprecations are ready)

[kddevin]

Thoughts on ReadOnly/WriteOnly/ReadWrite access:

1/29/21

namespace Access {
  struct ReadOnly  {};
  struct WriteOnly {};
  struct ReadWrite {};
}

getLocalView<device>(Tpetra::Access::ReadOnly());
getLocalViewHost(Tpetra::Access::WriteOnly());
getLocalViewDevice(Tpetra::Access::ReadWrite());

getLocalViewHost(const Tpetra::Access::WriteOnly &ignoreme) {
  don't bother syncing
  clear the sync flags
  mark host modify
  return host pointer
}

const getLocalViewHost(const Tpetra::Access::ReadOnly &ignoreme) const {
  sync if needed
  return host pointer
}

getLocalViewHost(const Tpetra::Access::ReadWrite &ignoreme) {
  sync if needed
  mark host modify
  return host pointer
}


getLocalView<device>(Access::ReadOnly());
getLocalViewHost(Access::WriteOnly());
getLocalViewDevice(Access::ReadWrite());

@kddevin will change existing Access struct in withLocalAccess
@brian-kelley will put the functions and tags n place
@trilinos/tpetra will update branch to use the new functions with tags

[kddevin]

@brian-kelley @csiefer2 I got all tests except the mv-reduce-strided test passing today.
The templated getLocalView<>(access) was not working correctly; it was always following the host view path. So it would return the correct view but with the flags set and sync done incorrectly. I changed the test for which path to take. It isn't an elegant test, though, so you probably should review it. 13e3a19
I also changed the implementation of getLocalViewHost and getLocalViewDevice to NOT call the templated version. From a design standpoint, I like avoiding the if tests in the templated function when the user has explicitly told us which view he wants. But I don't like having the logic duplicated in the templated and non-templated functions. I wish we could just specialize the templated function.
Let's discuss on Tuesday.

[kyungjoo-kim]

I removed the sync, modify things on the multivector but I leave the interface there. This interface can be removed later after other items are fixed ie., local access interface is eliminated.

[brian-kelley]

@kyungjoo-kim @kddevin I just checked in the removal of withLocalAccess, for_each and transform. The one issue I still see in Ifpack2 is that RBILUK is still trying to call removed BlockMultiVector functions like sync_host. But the things that used withLocalAccess (Chebyshev, Relaxation) built and passed before I rebased on that commit. (I mean with UVM)

[brian-kelley]

I got rid of all the deprecated MV/BlockMV calls in Ifpack2, they're all using the new interface.

There were 2 tests in MultiVector_LocalViewTests that failed with CudaUVMSpace: HostDeviceView and DeviceHostView. I think the issue is that they were expecting { h = mv.getLocalViewHost(); d = mv.getLocalViewDevice(); } to always throw, but it only throws if host and device are different spaces (if they're not, DualView's host and device views are the same, so reference counts are the same). I fixed one by doing this instead:

  bool shouldThrow = ! Kokkos::SpaceAccessibility<Kokkos::Serial, typename Node::memory_space>::accessible;

and that fixed HostDeviceView. DeviceHostView I commented out but the same fix should work there.

[brian-kelley]

Ifpack2 failures with UVM, without launch blocking:

• Ifpack2_RILUK_hb_belos_MPI_2
• Ifpack2_RILUK_hb_belos_MPI_4
• Ifpack2_unit_tests_MPI_4
• Ifpack2_AdditiveSchwarz_MPI_4
• Ifpack2_RBILUK_MPI_4
• Ifpack2_RILUKSingleProcessUnitTests_MPI_1
• Ifpack2_BlockTriDiContainerUnitAndPerfTests_MPI_4

[brian-kelley]

@kddevin With the last commit, all tests in Tpetra, Belos, Ifpack2, MueLu, and Teko pass with UVM and without launch blocking.

[kyungjoo-kim]

@brian-kelley Does the last commit mean that the fencing execution space in sync_host and sync_device ? If so, that does not make sense. When I and @crtrott talked, the execution fence in sync function is redundant as Kokkos::deep_copy already puts Kokkos::fence() when it has the same pointers. here To me, it is a mystery that doulbe fencing actually solves a problem when launch blocking is off.

@crtrott any idea ?

[jhux2]

Did someone say "double fencing"? @csiefer2 might have something to say about that.

[brian-kelley]

@jhux2 Luckily, this is a 0 vs. 1 fences being different, not a 1 vs. 2 fences.

@kyungjoo-kim You were right, even though the fencing was not consistent before, just adding them to sync_host and sync_device wasn't the correct solution. Have a look at kokkos/kokkos#3850 . I think our templated getLocalView<Device>(tag) is always taking the device path because DualView's type logic is inconsistent between view(), sync() and modify(). That means we could be skipping syncs and/or getting wrong modify flags.

[brian-kelley]

That's not the cause of all the ifpack2 failures, though.

[kyungjoo-kim]

@brian-kelley That is indeed a sneaky issue.

[brian-kelley]

I'm seeing some new failures without CUDA_LAUNCH_BLOCKING since we rebased on develop.

TpetraCore_CrsMatrix_Bug8794_MPI_4
TpetraCore_BlockCrsMatrix_MPI_4  -- this one is random, seems to fail roughly half the time
Ifpack2_RILUK_hb_belos_MPI_2
Ifpack2_RILUK_hb_belos_MPI_4
Ifpack2_AdditiveSchwarz_MPI_4
Ifpack2_RBILUK_MPI_4
Ifpack2_RILUKSingleProcessUnitTests_MPI_1

Update 4-13: only these two are failing for me now (cuda-unaware MPI, uvm, no launch block)

MueLu_UnitTestsIntrepid2Tpetra_MPI_1
MueLu_UnitTestsIntrepid2Tpetra_MPI_4

Update 4-16: The MueLu intrepid2 tests are passing for me after the last fix.