Skip to content
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.

Sign up for GitHub

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

Amesos2: Potential fix for issue #2270 #3300

Merged
merged 3 commits into from
Aug 17, 2018
Merged

Amesos2: Potential fix for issue #2270 #3300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c6d403
Amesos2: Potential fix for issue #2270
ndellingwood Aug 15, 2018
7e16491
Amesos2: Add fence for UVM in MV code, replace loop with std::memcpy
ndellingwood Aug 15, 2018
b8deb83
Amesos2: Fix element-wise copy in MVAdapter for possible fad types
ndellingwood Aug 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 11 additions & 20 deletions packages/amesos2/src/Amesos2_TpetraMultiVecAdapter_def.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
#ifndef AMESOS2_TPETRA_MULTIVEC_ADAPTER_DEF_HPP
#define AMESOS2_TPETRA_MULTIVEC_ADAPTER_DEF_HPP

#include <type_traits>
#include "Amesos2_TpetraMultiVecAdapter_decl.hpp"


Expand Down Expand Up @@ -211,23 +212,14 @@ namespace Amesos2 {
const size_t lclNumRows = redist_mv.getLocalLength();
for (size_t j = 0; j < redist_mv.getNumVectors(); ++j) {
auto av_j = av(lda*j, lclNumRows);
auto X_j = redist_mv.getVector(j);
auto X_lcl_j_2d = redist_mv.template getLocalView<host_execution_space> ();
auto X_lcl_j_1d = Kokkos::subview (X_lcl_j_2d, Kokkos::ALL (), j);
for ( size_t i = 0; i < lclNumRows; ++i ) {
av_j[i] = X_lcl_j_1d(i);
}

using val_type = typename decltype( X_lcl_j_1d )::value_type;
Kokkos::View<val_type*, Kokkos::HostSpace> umavj ( const_cast< val_type* > ( reinterpret_cast<const val_type*> ( av_j.getRawPtr () ) ), av_j.size () );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come av_j is const? Now is not the time to dig into that, though ;-) .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RHS MV (i.e. 'B') is stored as const in the MVAdapter (after being copied on root proc) to prevent modification, which is why av_j ends up being const.

Kokkos::deep_copy (umavj, X_lcl_j_1d);
}
}

auto global_contiguous_size = distMap->getGlobalNumElements(); //maybe use getGlobalLength() from the mv
auto local_contiguous_size = (distMap->getComm()->getRank() == 0) ? global_contiguous_size : 0;
RCP<const map_type> contigMap = rcp( new map_type(global_contiguous_size, local_contiguous_size, 0, distMap->getComm() ));

typedef Tpetra::Export<local_ordinal_t, global_ordinal_t, node_t> contiguous_export_t;
RCP<contiguous_export_t> contig_exporter = rcp( new contiguous_export_t(redist_mv.getMap(), contigMap) );
multivec_t contig_mv( contigMap, num_vecs);
contig_mv.doExport(redist_mv, *contig_exporter, Tpetra::INSERT);
}
}
}
Expand Down Expand Up @@ -335,7 +327,7 @@ namespace Amesos2 {
// num_vecs = 1; stride does not matter
auto mv_view_to_modify_2d = mv_->template getLocalView<host_execution_space>();
for ( size_t i = 0; i < lda; ++i ) {
mv_view_to_modify_2d(i,0) = new_data[i];
mv_view_to_modify_2d(i,0) = new_data[i]; // Only one vector
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do a deep_copy instead of writing a for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhoemmen thanks for feedback! This was code I mostly worked on during my first go at using Tpetra awhile back, I wasn't sure if deep_copy from an ArrayView (new_data) and a 1d View was supported - I'm supposing it must be based on the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just to clarify -- the only way to use deep_copy with ArrayView right now is to turn the ArrayView into an unowned HostSpace View. You did that above :) .)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll also want to fix this bit in the same way that you fixed it above. Also, if you know that the MultiVector has only one column, you could turn that 2-D View into a 1-D View first, before doing the copy.

}
}
else {
Expand All @@ -362,15 +354,14 @@ namespace Amesos2 {
srcMap = importer_->getSourceMap ();
}

multivec_t redist_mv (srcMap, num_vecs);

if ( distribution != CONTIGUOUS_AND_ROOTED ) {
// Do this if GIDs contiguous - existing functionality
// Redistribute the output (multi)vector.
const multivec_t source_mv (srcMap, new_data, lda, num_vecs);
mv_->doImport (source_mv, *importer_, Tpetra::REPLACE);
}
else {
multivec_t redist_mv (srcMap, num_vecs); // unused for ROOTED case
typedef typename multivec_t::dual_view_type dual_view_type;
typedef typename dual_view_type::host_mirror_space host_execution_space;
redist_mv.template modify< host_execution_space > ();
Expand All @@ -392,12 +383,12 @@ namespace Amesos2 {
const size_t lclNumRows = redist_mv.getLocalLength();
for (size_t j = 0; j < redist_mv.getNumVectors(); ++j) {
auto av_j = new_data(lda*j, lclNumRows);
auto X_j = redist_mv.getVector(j);
auto X_lcl_j_2d = redist_mv.template getLocalView<host_execution_space> ();
auto X_lcl_j_1d = Kokkos::subview (X_lcl_j_2d, Kokkos::ALL (), j);
for ( size_t i = 0; i < lclNumRows; ++i ) {
X_lcl_j_1d(i) = av_j[i];
}

using val_type = typename decltype( X_lcl_j_1d )::value_type;
Kokkos::View<val_type*, Kokkos::HostSpace> umavj ( const_cast< val_type* > ( reinterpret_cast<const val_type*> ( av_j.getRawPtr () ) ), av_j.size () );
Kokkos::deep_copy (umavj, X_lcl_j_1d);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This code begs for a refactor -- this function ("copy from an ArrayView to a MultiVector") seems to come up a few times.
  2. Does new_data come from a MultiVector? If so, you could just Tpetra::deep_copy between the two MultiVectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhoemmen: "this function ("copy from an ArrayView to a MultiVector") seems to come up a few times"

Yeah, we've reimplemented some optimizations from Amesos to avoid this for single process cases when the data pointer can be used directly without worrying about collectives to bring all data to the root proc. Adding @srajama1, we can discuss some possible options, estimate amount of work, availability etc.

"Does new_data come from a MultiVector"
No, new_data is an ArrayView storing the result (on root proc) of the solve. It needs to be copied back and redistributed to the MultiVector passed in through the solve(...) call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndellingwood : Can you file a different issue for optimizing this code. Let us get the PR with the current changes pushed in and we can look at optimizing this as a next step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srajama1 sounds good :)

}
}

Expand Down
8 changes: 4 additions & 4 deletions packages/amesos2/test/solvers/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ TRIBITS_ADD_EXECUTABLE(

##### KLU2 Tests #####

IF (${PACKAGE_NAME}_ENABLE_KLU2 AND NOT ${PROJECT_NAME}_ENABLE_DEBUG AND NOT KOKKOS_ENABLE_DEBUG)
##IF (${PACKAGE_NAME}_ENABLE_KLU2 AND NOT ${PROJECT_NAME}_ENABLE_DEBUG AND NOT KOKKOS_ENABLE_DEBUG)
IF (${PACKAGE_NAME}_ENABLE_KLU2)
TRIBITS_ADD_EXECUTABLE_AND_TEST(
KLU2_UnitTests
# EXCLUDE_IF_NOT_TRUE ${PROJECT_NAME}_ENABLE_DEBUG
Expand Down Expand Up @@ -188,8 +189,8 @@ ENDIF()


#### BASKER Tests ####
IF (${PACKAGE_NAME}_ENABLE_Basker AND NOT ${PROJECT_NAME}_ENABLE_DEBUG)

## IF (${PACKAGE_NAME}_ENABLE_Basker AND NOT ${PROJECT_NAME}_ENABLE_DEBUG)
IF (${PACKAGE_NAME}_ENABLE_Basker)
TRIBITS_ADD_EXECUTABLE_AND_TEST(
Basker_UnitTests
SOURCES
Expand All @@ -204,7 +205,6 @@ ENDIF()

#### SHYLUBASKER Tests ####
IF (${PACKAGE_NAME}_ENABLE_ShyLU_NodeBasker)

TRIBITS_ADD_EXECUTABLE_AND_TEST(
ShyLUBasker_UnitTests
SOURCES
Expand Down