Skip to content

Commit

Permalink
review updates and interface improvements
Browse files Browse the repository at this point in the history 
- pull in convert_to/move_to pointer_param overloads
- make Executor::copy_from work on shared_ptr
- remove unnecessary add_scaled_identity overload
- simplify gko::write
- deprecate moving copy_from
- simplify move_from
- make temporary_clone/conversion work on pointer_param
- make make_(const_)dense_view work on pointer_param
- make stopping criterion updater work on pointer_param
- add documentation
- fix accidentally deleted copy constructor
- deprecate moving copy_from

Co-authored-by: Thomas Grützmacher <thomas.gruetzmacher@kit.edu>
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
  • Loading branch information
@upsj @MarcelKoch
3 people committed Feb 15, 2023
1 parent 26d6fea commit e0c7983
Show file tree
Hide file tree
Showing 15 changed files with 215 additions and 199 deletions.
9 changes: 5 additions & 4 deletions core/distributed/matrix.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,16 @@ Matrix<ValueType, LocalIndexType, GlobalIndexType>::Matrix(
template <typename ValueType, typename LocalIndexType, typename GlobalIndexType>
Matrix<ValueType, LocalIndexType, GlobalIndexType>::Matrix(
std::shared_ptr<const Executor> exec, mpi::communicator comm,
const LinOp* local_matrix_type)
pointer_param<const LinOp> local_matrix_type)
: Matrix(exec, comm, local_matrix_type, local_matrix_type)
{}


template <typename ValueType, typename LocalIndexType, typename GlobalIndexType>
Matrix<ValueType, LocalIndexType, GlobalIndexType>::Matrix(
std::shared_ptr<const Executor> exec, mpi::communicator comm,
const LinOp* local_matrix_template, const LinOp* non_local_matrix_template)
pointer_param<const LinOp> local_matrix_template,
pointer_param<const LinOp> non_local_matrix_template)
: EnableDistributedLinOp<
Matrix<value_type, local_index_type, global_index_type>>{exec},
DistributedBase{comm},
Expand Down Expand Up @@ -173,8 +174,8 @@ void Matrix<ValueType, LocalIndexType, GlobalIndexType>::read_distributed(

// build local, non-local matrix data and communication structures
exec->run(matrix::make_build_local_nonlocal(
data, make_temporary_clone(exec, row_partition.get()).get(),
make_temporary_clone(exec, col_partition.get()).get(), local_part,
data, make_temporary_clone(exec, row_partition).get(),
make_temporary_clone(exec, col_partition).get(), local_part,
local_row_idxs, local_col_idxs, local_values, non_local_row_idxs,
non_local_col_idxs, non_local_values, recv_gather_idxs,
recv_sizes_array, non_local_to_global_));
Expand Down
69 changes: 33 additions & 36 deletions core/matrix/dense.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,15 +156,15 @@ template <typename ValueType>
void Dense<ValueType>::scale(pointer_param<const LinOp> alpha)
{
auto exec = this->get_executor();
this->scale_impl(make_temporary_clone(exec, alpha.get()).get());
this->scale_impl(make_temporary_clone(exec, alpha).get());
}


template <typename ValueType>
void Dense<ValueType>::inv_scale(pointer_param<const LinOp> alpha)
{
auto exec = this->get_executor();
this->inv_scale_impl(make_temporary_clone(exec, alpha.get()).get());
this->inv_scale_impl(make_temporary_clone(exec, alpha).get());
}


Expand All @@ -173,8 +173,8 @@ void Dense<ValueType>::add_scaled(pointer_param<const LinOp> alpha,
pointer_param<const LinOp> b)
{
auto exec = this->get_executor();
this->add_scaled_impl(make_temporary_clone(exec, alpha.get()).get(),
make_temporary_clone(exec, b.get()).get());
this->add_scaled_impl(make_temporary_clone(exec, alpha).get(),
make_temporary_clone(exec, b).get());
}


Expand All @@ -183,8 +183,8 @@ void Dense<ValueType>::sub_scaled(pointer_param<const LinOp> alpha,
pointer_param<const LinOp> b)
{
auto exec = this->get_executor();
this->sub_scaled_impl(make_temporary_clone(exec, alpha.get()).get(),
make_temporary_clone(exec, b.get()).get());
this->sub_scaled_impl(make_temporary_clone(exec, alpha).get(),
make_temporary_clone(exec, b).get());
}


Expand All @@ -193,9 +193,8 @@ void Dense<ValueType>::compute_dot(pointer_param<const LinOp> b,
pointer_param<LinOp> result) const
{
auto exec = this->get_executor();
this->compute_dot_impl(
make_temporary_clone(exec, b.get()).get(),
make_temporary_output_clone(exec, result.get()).get());
this->compute_dot_impl(make_temporary_clone(exec, b).get(),
make_temporary_output_clone(exec, result).get());
}


Expand All @@ -205,26 +204,24 @@ void Dense<ValueType>::compute_conj_dot(pointer_param<const LinOp> b,
{
auto exec = this->get_executor();
this->compute_conj_dot_impl(
make_temporary_clone(exec, b.get()).get(),
make_temporary_output_clone(exec, result.get()).get());
make_temporary_clone(exec, b).get(),
make_temporary_output_clone(exec, result).get());
}


template <typename ValueType>
void Dense<ValueType>::compute_norm2(pointer_param<LinOp> result) const
{
auto exec = this->get_executor();
this->compute_norm2_impl(
make_temporary_output_clone(exec, result.get()).get());
this->compute_norm2_impl(make_temporary_output_clone(exec, result).get());
}


template <typename ValueType>
void Dense<ValueType>::compute_norm1(pointer_param<LinOp> result) const
{
auto exec = this->get_executor();
this->compute_norm1_impl(
make_temporary_output_clone(exec, result.get()).get());
this->compute_norm1_impl(make_temporary_output_clone(exec, result).get());
}


Expand Down Expand Up @@ -355,10 +352,10 @@ void Dense<ValueType>::compute_dot(pointer_param<const LinOp> b,
tmp.clear();
tmp.set_executor(exec);
}
auto local_b = make_temporary_clone(exec, b.get());
auto local_res = make_temporary_clone(exec, result.get());
auto dense_b = make_temporary_conversion<ValueType>(local_b.get());
auto dense_res = make_temporary_conversion<ValueType>(local_res.get());
auto local_b = make_temporary_clone(exec, b);
auto local_res = make_temporary_clone(exec, result);
auto dense_b = make_temporary_conversion<ValueType>(local_b);
auto dense_res = make_temporary_conversion<ValueType>(local_res);
exec->run(
dense::make_compute_dot(this, dense_b.get(), dense_res.get(), tmp));
}
Expand Down Expand Up @@ -390,10 +387,10 @@ void Dense<ValueType>::compute_conj_dot(pointer_param<const LinOp> b,
tmp.clear();
tmp.set_executor(exec);
}
auto local_b = make_temporary_clone(exec, b.get());
auto local_res = make_temporary_clone(exec, result.get());
auto dense_b = make_temporary_conversion<ValueType>(local_b.get());
auto dense_res = make_temporary_conversion<ValueType>(local_res.get());
auto local_b = make_temporary_clone(exec, b);
auto local_res = make_temporary_clone(exec, result);
auto dense_b = make_temporary_conversion<ValueType>(local_b);
auto dense_res = make_temporary_conversion<ValueType>(local_res);
exec->run(dense::make_compute_conj_dot(this, dense_b.get(), dense_res.get(),
tmp));
}
Expand Down Expand Up @@ -424,7 +421,7 @@ void Dense<ValueType>::compute_norm2(pointer_param<LinOp> result,
tmp.clear();
tmp.set_executor(exec);
}
auto local_result = make_temporary_clone(exec, result.get());
auto local_result = make_temporary_clone(exec, result);
auto dense_res = make_temporary_conversion<remove_complex<ValueType>>(
local_result.get());
exec->run(dense::make_compute_norm2(this, dense_res.get(), tmp));
Expand Down Expand Up @@ -453,7 +450,7 @@ void Dense<ValueType>::compute_norm1(pointer_param<LinOp> result,
tmp.clear();
tmp.set_executor(exec);
}
auto local_result = make_temporary_clone(exec, result.get());
auto local_result = make_temporary_clone(exec, result);
auto dense_res = make_temporary_conversion<remove_complex<ValueType>>(
local_result.get());
exec->run(dense::make_compute_norm1(this, dense_res.get(), tmp));
Expand Down Expand Up @@ -1026,7 +1023,7 @@ void Dense<ValueType>::transpose(pointer_param<Dense<ValueType>> output) const
GKO_ASSERT_EQUAL_DIMENSIONS(output, gko::transpose(this->get_size()));
auto exec = this->get_executor();
exec->run(dense::make_transpose(
this, make_temporary_output_clone(exec, output.get()).get()));
this, make_temporary_output_clone(exec, output).get()));
}


Expand All @@ -1037,7 +1034,7 @@ void Dense<ValueType>::conj_transpose(
GKO_ASSERT_EQUAL_DIMENSIONS(output, gko::transpose(this->get_size()));
auto exec = this->get_executor();
exec->run(dense::make_conj_transpose(
this, make_temporary_output_clone(exec, output.get()).get()));
this, make_temporary_output_clone(exec, output).get()));
}


Expand Down Expand Up @@ -1344,8 +1341,8 @@ void Dense<ValueType>::row_gather(pointer_param<const LinOp> alpha,
pointer_param<const LinOp> beta,
pointer_param<LinOp> out) const
{
auto dense_alpha = make_temporary_conversion<ValueType>(alpha.get());
auto dense_beta = make_temporary_conversion<ValueType>(beta.get());
auto dense_alpha = make_temporary_conversion<ValueType>(alpha);
auto dense_beta = make_temporary_conversion<ValueType>(beta);
GKO_ASSERT_EQUAL_DIMENSIONS(dense_alpha, gko::dim<2>(1, 1));
GKO_ASSERT_EQUAL_DIMENSIONS(dense_beta, gko::dim<2>(1, 1));
gather_mixed_real_complex<ValueType>(
Expand All @@ -1362,8 +1359,8 @@ void Dense<ValueType>::row_gather(pointer_param<const LinOp> alpha,
pointer_param<const LinOp> beta,
pointer_param<LinOp> out) const
{
auto dense_alpha = make_temporary_conversion<ValueType>(alpha.get());
auto dense_beta = make_temporary_conversion<ValueType>(beta.get());
auto dense_alpha = make_temporary_conversion<ValueType>(alpha);
auto dense_beta = make_temporary_conversion<ValueType>(beta);
GKO_ASSERT_EQUAL_DIMENSIONS(dense_alpha, gko::dim<2>(1, 1));
GKO_ASSERT_EQUAL_DIMENSIONS(dense_beta, gko::dim<2>(1, 1));
gather_mixed_real_complex<ValueType>(
Expand Down Expand Up @@ -1498,7 +1495,7 @@ void Dense<ValueType>::extract_diagonal(
GKO_ASSERT_EQ(output->get_size()[0], diag_size);

exec->run(dense::make_extract_diagonal(
this, make_temporary_output_clone(exec, output.get()).get()));
this, make_temporary_output_clone(exec, output).get()));
}


Expand Down Expand Up @@ -1538,7 +1535,7 @@ void Dense<ValueType>::compute_absolute(
auto exec = this->get_executor();

exec->run(dense::make_outplace_absolute_dense(
this, make_temporary_output_clone(exec, output.get()).get()));
this, make_temporary_output_clone(exec, output).get()));
}


Expand All @@ -1559,7 +1556,7 @@ void Dense<ValueType>::make_complex(pointer_param<complex_type> result) const
auto exec = this->get_executor();

exec->run(dense::make_make_complex(
this, make_temporary_output_clone(exec, result.get()).get()));
this, make_temporary_output_clone(exec, result).get()));
}


Expand All @@ -1580,7 +1577,7 @@ void Dense<ValueType>::get_real(pointer_param<real_type> result) const
auto exec = this->get_executor();

exec->run(dense::make_get_real(
this, make_temporary_output_clone(exec, result.get()).get()));
this, make_temporary_output_clone(exec, result).get()));
}


Expand All @@ -1601,7 +1598,7 @@ void Dense<ValueType>::get_imag(pointer_param<real_type> result) const
auto exec = this->get_executor();

exec->run(dense::make_get_imag(
this, make_temporary_output_clone(exec, result.get()).get()));
this, make_temporary_output_clone(exec, result).get()));
}


Expand Down
6 changes: 5 additions & 1 deletion core/test/base/mtx_io.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1087,12 +1087,16 @@ TYPED_TEST(RealDummyLinOpTest, WritesLinOpToStreamDefault)
auto lin_op = gko::read<DummyLinOp<value_type, index_type>>(
iss, gko::ReferenceExecutor::create());
std::ostringstream oss{};
std::ostringstream oss_const{};

write(oss, lend(lin_op));
write(oss, lin_op);
write(oss_const, std::unique_ptr<const DummyLinOp<value_type, index_type>>{
std::move(lin_op)});

ASSERT_EQ(oss.str(),
"%%MatrixMarket matrix coordinate real general\n2 3 6\n1 1 1\n1 "
"2 3\n1 3 2\n2 1 0\n2 2 5\n2 3 0\n");
ASSERT_EQ(oss_const.str(), oss.str());
}


Expand Down
20 changes: 11 additions & 9 deletions include/ginkgo/core/base/executor.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -765,19 +765,19 @@ class Executor : public log::EnableLogging<Executor> {
* where the data will be copied to
*/
template <typename T>
void copy_from(const Executor* src_exec, size_type num_elems,
void copy_from(pointer_param<const Executor> src_exec, size_type num_elems,
const T* src_ptr, T* dest_ptr) const
{
const auto src_loc = reinterpret_cast<uintptr>(src_ptr);
const auto dest_loc = reinterpret_cast<uintptr>(dest_ptr);
this->template log<log::Logger::copy_started>(
src_exec, this, src_loc, dest_loc, num_elems * sizeof(T));
if (this != src_exec) {
src_exec.get(), this, src_loc, dest_loc, num_elems * sizeof(T));
if (this != src_exec.get()) {
src_exec->template log<log::Logger::copy_started>(
src_exec, this, src_loc, dest_loc, num_elems * sizeof(T));
src_exec.get(), this, src_loc, dest_loc, num_elems * sizeof(T));
}
try {
this->raw_copy_from(src_exec, num_elems * sizeof(T), src_ptr,
this->raw_copy_from(src_exec.get(), num_elems * sizeof(T), src_ptr,
dest_ptr);
} catch (NotSupported&) {
#if (GKO_VERBOSE_LEVEL >= 1) && !defined(NDEBUG)
Expand All @@ -787,7 +787,7 @@ class Executor : public log::EnableLogging<Executor> {
<< std::endl;
#endif
auto src_master = src_exec->get_master().get();
if (num_elems > 0 && src_master != src_exec) {
if (num_elems > 0 && src_master != src_exec.get()) {
auto* master_ptr = src_exec->get_master()->alloc<T>(num_elems);
src_master->copy_from<T>(src_exec, num_elems, src_ptr,
master_ptr);
Expand All @@ -796,10 +796,10 @@ class Executor : public log::EnableLogging<Executor> {
}
}
this->template log<log::Logger::copy_completed>(
src_exec, this, src_loc, dest_loc, num_elems * sizeof(T));
if (this != src_exec) {
src_exec.get(), this, src_loc, dest_loc, num_elems * sizeof(T));
if (this != src_exec.get()) {
src_exec->template log<log::Logger::copy_completed>(
src_exec, this, src_loc, dest_loc, num_elems * sizeof(T));
src_exec.get(), this, src_loc, dest_loc, num_elems * sizeof(T));
}
}

Expand Down Expand Up @@ -879,6 +879,8 @@ class Executor : public log::EnableLogging<Executor> {
this->EnableLogging<Executor>::remove_logger(logger);
}

using EnableLogging<Executor>::remove_logger;

/**
* Sets the logger event propagation mode for the executor.
* This controls whether events that happen at objects created on this
Expand Down
33 changes: 14 additions & 19 deletions include/ginkgo/core/base/lin_op.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ class LinOp : public EnableAbstractPolymorphicObject<LinOp> {
x.get());
this->validate_application_parameters(b.get(), x.get());
auto exec = this->get_executor();
this->apply_impl(make_temporary_clone(exec, b.get()).get(),
make_temporary_clone(exec, x.get()).get());
this->apply_impl(make_temporary_clone(exec, b).get(),
make_temporary_clone(exec, x).get());
this->template log<log::Logger::linop_apply_completed>(this, b.get(),
x.get());
return this;
Expand All @@ -178,8 +178,8 @@ class LinOp : public EnableAbstractPolymorphicObject<LinOp> {
x.get());
this->validate_application_parameters(b.get(), x.get());
auto exec = this->get_executor();
this->apply_impl(make_temporary_clone(exec, b.get()).get(),
make_temporary_clone(exec, x.get()).get());
this->apply_impl(make_temporary_clone(exec, b).get(),
make_temporary_clone(exec, x).get());
this->template log<log::Logger::linop_apply_completed>(this, b.get(),
x.get());
return this;
Expand All @@ -203,10 +203,10 @@ class LinOp : public EnableAbstractPolymorphicObject<LinOp> {
this->validate_application_parameters(alpha.get(), b.get(), beta.get(),
x.get());
auto exec = this->get_executor();
this->apply_impl(make_temporary_clone(exec, alpha.get()).get(),
make_temporary_clone(exec, b.get()).get(),
make_temporary_clone(exec, beta.get()).get(),
make_temporary_clone(exec, x.get()).get());
this->apply_impl(make_temporary_clone(exec, alpha).get(),
make_temporary_clone(exec, b).get(),
make_temporary_clone(exec, beta).get(),
make_temporary_clone(exec, x).get());
this->template log<log::Logger::linop_advanced_apply_completed>(
this, alpha.get(), b.get(), beta.get(), x.get());
return this;
Expand All @@ -225,10 +225,10 @@ class LinOp : public EnableAbstractPolymorphicObject<LinOp> {
this->validate_application_parameters(alpha.get(), b.get(), beta.get(),
x.get());
auto exec = this->get_executor();
this->apply_impl(make_temporary_clone(exec, alpha.get()).get(),
make_temporary_clone(exec, b.get()).get(),
make_temporary_clone(exec, beta.get()).get(),
make_temporary_clone(exec, x.get()).get());
this->apply_impl(make_temporary_clone(exec, alpha).get(),
make_temporary_clone(exec, b).get(),
make_temporary_clone(exec, beta).get(),
make_temporary_clone(exec, x).get());
this->template log<log::Logger::linop_advanced_apply_completed>(
this, alpha.get(), b.get(), beta.get(), x.get());
return this;
Expand Down Expand Up @@ -838,7 +838,8 @@ class ScaledIdentityAddable {
* @param b Scalar to multiply this before adding the scaled identity to
* it.
*/
void add_scaled_identity(const LinOp* const a, const LinOp* const b)
void add_scaled_identity(pointer_param<const LinOp> const a,
pointer_param<const LinOp> const b)
{
GKO_ASSERT_IS_SCALAR(a);
GKO_ASSERT_IS_SCALAR(b);
Expand All @@ -847,12 +848,6 @@ class ScaledIdentityAddable {
add_scaled_identity_impl(ae.get(), be.get());
}

void add_scaled_identity(pointer_param<const LinOp> const a,
pointer_param<const LinOp> const b)
{
add_scaled_identity(a.get(), b.get());
}

private:
virtual void add_scaled_identity_impl(const LinOp* a, const LinOp* b) = 0;
};
Expand Down
Loading

0 comments on commit e0c7983

Please sign in to comment.