-
Notifications
You must be signed in to change notification settings - Fork 578
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
Tpetra: Don't access DualView's [h|d]_view directly #13778
Tpetra: Don't access DualView's [h|d]_view directly #13778
Conversation
Signed-off-by: Daniel Arndt <[email protected]>
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
// DEEP_COPY REVIEW - DEVICE-TO-HOSTMIRROR | ||
Kokkos::deep_copy (execution_space(), devView, hostView); | ||
dv = dual_view_type (devView, hostView); | ||
execution_space().fence(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fence is new but seems necessary. The constructor will not fence and the deep_copy
is asynchronous. The function doesn't take an execution space instance argument so it should be synchronous.
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: masterleinad |
@@ -29,8 +29,8 @@ | |||
if (envVarSet && (std::strcmp(envVarSet,"1") == 0)) \ | |||
std::cout << (fn) << " called from " << callerstr \ | |||
<< " at " << filestr << ":"<<linnum \ | |||
<< " host cnt " << dualView.h_view.use_count() \ | |||
<< " device cnt " << dualView.d_view.use_count() \ | |||
<< " host cnt " << dualView.view_host().use_count() \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure if it’s an issue, but here, behavior changes, right? Because the getter is making a copy, so we’ll now report the previous use count incremented by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in general all use counts will be larger but from what I understand only the difference between host and device is of relevance here.
@@ -210,7 +210,7 @@ class WrappedDualView { | |||
} | |||
|
|||
size_t extent(const int i) const { | |||
return dualView.h_view.extent(i); | |||
return dualView.view_host().extent(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably become just dualView.extent(i)
.
@@ -551,11 +551,11 @@ class WrappedDualView { | |||
} | |||
|
|||
int host_view_use_count() const { | |||
return originalDualView.h_view.use_count(); | |||
return originalDualView.view_host().use_count(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, the increment by one may be worth considering. Similar cases below.
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
Signed-off-by: Daniel Arndt <[email protected]>
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: masterleinad |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
@masterleinad Are there any performance implications of this? |
Currently, we are returning by value in |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
4 similar comments
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ csiefer2 ]! |
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: masterleinad |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
Various nightly failures with kokkos@develop following merge of these changes, e.g. https://trilinos-cdash.sandia.gov/builds/2113214 Sample failure, appears to be common between various unit tests
I'm not seeing it with Cuda builds, but happens with the other backends |
@ndellingwood Can you provide a sample configuration? |
@masterleinad this is a sample reproducer run on the OHPC machine blake, so the module use would need to be modified on a different machine; I think enabling MPI in the build may be necessary to reproduce the failures # Environment
module purge
module load cmake gcc/11.3.0 openmpi/4.1.5 openblas/0.3.23
# Repo prep
git clone -b develop https://github.com/trilinos/Trilinos.git
TRILINOS_DIR=$PWD/Trilinos
git clone -b develop https://github.com/kokkos/kokkos.git
KOKKOS_DIR=$PWD/kokkos
git clone -b develop https://github.com/kokkos/kokkos-kernels.git
KOKKOSKERNELS_DIR=$PWD/kokkos-kernels
cd $TRILINOS_DIR
ln -s ${KOKKOS_DIR}/kokkos kokkos
ln -s ${KOKKOSKERNELS_DIR}/kokkos-kernels kokkos-kernels
cd -
# Create Kokkos, KokkosKernels, and Trilinos Build dirs
export BUILDDIR=${WORKDIR}/Build
mkdir -p $BUILDDIR
cd $BUILDDIR
export KOKKOS_BUILD=$BUILDDIR/Build_Kokkos
mkdir -p $KOKKOS_BUILD
export KOKKOSKERNELS_BUILD=$BUILDDIR/Build_KokkosKernels
mkdir -p $KOKKOSKERNELS_BUILD
export TRILINOS_BUILD=$BUILDDIR/Build_Trilinos
mkdir -p $TRILINOS_BUILD
cd $TRILINOS_BUILD
cmake \
-D CMAKE_CXX_COMPILER="`which mpicxx`" \
-D CMAKE_C_COMPILER="`which mpicc`" \
-D CMAKE_Fortran_COMPILER="`which mpifort`" \
-D CMAKE_CXX_FLAGS="-g" \
-D CMAKE_C_FLAGS="-g" \
-DTPL_ENABLE_MPI=ON \
-DTPL_ENABLE_BLAS:BOOL=ON \
-DBLAS_LIBRARY_DIRS:FILEPATH=${OPENBLAS_ROOT}/lib \
-DBLAS_LIBRARY_NAMES:STRING="openblas" \
-DTPL_ENABLE_LAPACK:BOOL=ON \
-DLAPACK_INCLUDE_DIRS:FILEPATH=${OPENBLAS_ROOT}/include \
-DLAPACK_LIBRARY_DIRS:FILEPATH=${OPENBLAS_ROOT}/lib \
-DLAPACK_LIBRARY_NAMES:STRING="openblas" \
-DTPL_ENABLE_Matio=OFF \
\
-DTrilinos_ENABLE_ALL_PACKAGES=OFF \
-DTrilinos_ENABLE_ALL_OPTIONAL_PACKAGES=OFF \
-DTrilinos_ENABLE_TESTS=ON \
-DTrilinos_MUST_FIND_ALL_TPL_LIBS=TRUE \
-DTrilinos_ENABLE_COMPLEX=ON \
-DTrilinos_ENABLE_OpenMP=ON \
-DTrilinos_ENABLE_Kokkos=ON \
-D Kokkos_ENABLE_OPENMP=ON \
-D Kokkos_ARCH_SKX=ON \
-DTrilinos_ENABLE_KokkosKernels=ON \
-DTrilinos_ENABLE_Tpetra=ON \
-D Tpetra_ENABLE_TESTS=ON \
$TRILINOS_DIR
# Build and test
make -j16
ctest -R Tpetra* --output-on-failure |
This reverts commit 9a9ba84. Trilinos performance dashboard shows regressions after this PR merged.
This reverts commit 9a9ba84. Trilinos performance dashboard shows regressions after this PR merged. Signed-off-by: Jonathan Hu <[email protected]>
Regressions observed in Tpetra FE Assembly test on Hops and Vortex due to PR #13778. See #13820. Signed-off-by: Jonathan Hu <[email protected]>
Regressions observed in Tpetra FE Assembly test on Hops and Vortex introduced by PR #13778. See #13820. Patch suggested by Daniel Arndt. Signed-off-by: Jonathan Hu <[email protected]>
@trilinos/tpetra
Motivation
kokkos/kokkos#7716 proposes to deprecate the ability to access
DualView
'sh_view
andd_view
members directly since modifying the allocations can get perturbed theDualView
's internal status. This pull request replaces all places inTpetra
that use these members directly, in most places, verbatim withview_device
resp.view_host
.Related Issues
Related to kokkos/kokkos#7716.
Testing
Compiling Trilinos with
Tpetra
enabled with the OpenMP backend and running theTpetra
tests.Signed-off-by: Daniel Arndt [email protected]