Allow passing smart pointers to functions directly (interface changes only) #1279

upsj · 2023-02-15T09:57:34Z

To simplify reviewing #1261, this contains only the public interface changes

MarcelKoch

Since there are no new additions compared to the other PR, as far as I can see, I just copy my review.

thoasm

LGTM!
for some nits, see my comments in PR #1261:
#1261 (review)

yhmtsai

how about to use pointer or raw_pointer?
the main concern is as on pointer_param from unique_ptr

CMakeLists.txt

yhmtsai · 2023-02-16T14:32:29Z

core/distributed/vector.cpp

+    const device_matrix_data<ValueType, int64>& data,
+    pointer_param<const Partition<int64, int64>> partition)


using the same thing like template <typename LocalIndexType, typename GlobalIndexType>?

like with gko::as(unique_ptr), templated functions can't be used here, because their parameters need to be an exact match, no implicit conversions allowed.

core/matrix/dense.cpp

include/ginkgo/core/base/executor.hpp

include/ginkgo/core/base/polymorphic_object.hpp

include/ginkgo/core/base/utils_helper.hpp

include/ginkgo/core/distributed/vector.hpp

include/ginkgo/core/matrix/coo.hpp

yhmtsai · 2023-02-16T15:11:02Z

include/ginkgo/core/matrix/dense.hpp

+    using ConvertibleTo<SparsityCsr<ValueType, int64>>::convert_to;
+    using ConvertibleTo<SparsityCsr<ValueType, int64>>::move_to;


could we mark convert_to_impl virtual and convert_to using the convert_to_impl in ConvertiableTo? (convert_to still need to be virtual to keep public interface)
then all class override convert_to_impl not convert_to

This seems more complicated to me than the current solution. This is only temporary anyways, until we can change the virtual functions to take pointer_param directly with the next major release.

yhmtsai · 2023-02-16T15:12:15Z

include/ginkgo/core/matrix/dense.hpp

-    void row_gather(const array<int32>* gather_indices,
-                    Dense* row_collection) const;


where be this moved to?

pointer_param<LinOp> accepts Dense*, so this overload is no longer necessary

I think the original one also accepts Dense*, but it can avoid casting to LinOp* and recasting back the Dense?

Basically, we can't have two overloads that take pointer_param<Dense> and pointer_param<LinOp>, because the call would then be ambiguous for any Dense pointer we pass in. The reason for this is that with raw pointers, more generic types (LinOp) are a worse fit than more specific types (Dense), but this is not true if we have converting constructors like the ones for pointer_param(T*)

With pointer_param, we will only allow the BaseType as the public interface, and then we need to dynamic_cast to the accepted types.

That is true, though dynamic_cast hasn't really been a performance issue so far. I think in the long term (Ginkgo 2.0), we should aim for making the parameter a Vector base class, instead of the current overloads being either too specific Dense<double> or too generic LinOp

after checking https://godbolt.org/z/8bcbYK8G4 again, the shared_ptr does not work, too. It only work when have derived and base function signature

yes, we always need exact matches, otherwise the ambiguity will come into play

MarcelKoch · 2023-02-17T14:03:00Z

Could raw_ptr as a type name make sense? Or raw_ptr_param? That would clear up the ownership issue. But then it's not clear that you can pass in a unique_ptr or a shared_ptr.

core/distributed/matrix.cpp

yhmtsai

LGTM in general. For the existed user LinOp, they are not required to put the using ConvertiableTo<UserLinOp>::convert_to, right? The UserLinOp works as origin but need the lend or .get.

yhmtsai · 2023-02-20T08:36:58Z

include/ginkgo/core/matrix/dense.hpp

-    void row_gather(const array<int32>* gather_indices,
-                    Dense* row_collection) const;


I think the original one also accepts Dense*, but it can avoid casting to LinOp* and recasting back the Dense?

yhmtsai · 2023-02-20T08:42:15Z

include/ginkgo/core/matrix/dense.hpp

+template <typename VecPtr>
+std::unique_ptr<
+    const matrix::Dense<typename detail::pointee<VecPtr>::value_type>>
+make_const_dense_view(VecPtr&& vector)


side note: Because create_const_view_of works only for Dense, it should be fine.
Otherwise, it can accept any type with value_type

upsj · 2023-02-20T08:49:02Z

@yhmtsai exactly, only the pointer_param overloads are missing if you omit the using statements

codecov · 2023-02-21T15:33:46Z

Codecov Report

Base: 91.24% // Head: 91.25% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (14b7f69) compared to base (e0ce4a9).
Patch coverage: 97.95% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1279   +/-   ##
========================================
  Coverage    91.24%   91.25%           
========================================
  Files          565      565           
  Lines        48026    48080   +54     
========================================
+ Hits         43822    43876   +54     
  Misses        4204     4204

Impacted Files	Coverage Δ
include/ginkgo/core/distributed/matrix.hpp	`100.00% <ø> (ø)`
include/ginkgo/core/distributed/vector.hpp	`33.33% <ø> (ø)`
include/ginkgo/core/log/logger.hpp	`96.55% <0.00%> (ø)`
include/ginkgo/core/matrix/ell.hpp	`100.00% <ø> (ø)`
include/ginkgo/core/matrix/fbcsr.hpp	`92.30% <ø> (ø)`
include/ginkgo/core/matrix/hybrid.hpp	`96.72% <ø> (ø)`
include/ginkgo/core/matrix/sellp.hpp	`90.24% <ø> (ø)`
include/ginkgo/core/matrix/sparsity_csr.hpp	`93.10% <ø> (ø)`
include/ginkgo/core/solver/solver_base.hpp	`79.05% <ø> (ø)`
include/ginkgo/core/base/temporary_conversion.hpp	`82.14% <66.66%> (ø)`
... and 31 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

upsj · 2023-02-21T15:51:31Z

rebase!

- 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>

- add tests for pointer_param - replace decay::type by decay_t - fix documentation Co-authored-by: Yen-Chen Chen <yen-chen.chen@kit.edu> Co-authored-by: Thomas Grützmacher <thomas.gruetzmacher@kit.edu>

Co-authored-by: Yuhsiang M. Tsai <yhmtsai@gmail.com>

ginkgo-bot · 2023-02-21T16:25:21Z

Note: This PR changes the Ginkgo ABI:

Functions changes summary: 1059 Removed, 186 Changed (25619 filtered out), 1780 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

For details check the full ABI diff under Artifacts here

sonarqubecloud · 2023-02-22T05:46:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
27 Code Smells

86.5% Coverage
8.8% Duplication

Release 1.6.0 of Ginkgo. The Ginkgo team is proud to announce the new Ginkgo minor release 1.6.0. This release brings new features such as: - Several building blocks for GPU-resident sparse direct solvers like symbolic and numerical LU and Cholesky factorization, ..., - A distributed Schwarz preconditioner, - New FGMRES and GCR solvers, - Distributed benchmarks for the SpMV operation, solvers, ... - Support for non-default streams in the CUDA and HIP backends, - Mixed precision support for the CSR SpMV, - A new profiling logger which integrates with NVTX, ROCTX, TAU and VTune to provide internal Ginkgo knowledge to most HPC profilers! and much more. If you face an issue, please first check our [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues) and the [open issues list](https://github.com/ginkgo-project/ginkgo/issues) and if you do not find a solution, feel free to [open a new issue](https://github.com/ginkgo-project/ginkgo/issues/new/choose) or ask a question using the [github discussions](https://github.com/ginkgo-project/ginkgo/discussions). Supported systems and requirements: + For all platforms, CMake 3.13+ + C++14 compliant compiler + Linux and macOS + GCC: 5.5+ + clang: 3.9+ + Intel compiler: 2018+ + Apple Clang: 14.0 is tested. Earlier versions might also work. + NVHPC: 22.7+ + Cray Compiler: 14.0.1+ + CUDA module: CUDA 9.2+ or NVHPC 22.7+ + HIP module: ROCm 4.5+ + DPC++ module: Intel OneAPI 2021.3+ with oneMKL and oneDPL. Set the CXX compiler to `dpcpp`. + Windows + MinGW: GCC 5.5+ + Microsoft Visual Studio: VS 2019+ + CUDA module: CUDA 9.2+, Microsoft Visual Studio + OpenMP module: MinGW. ### Version Support Changes + ROCm 4.0+ -> 4.5+ after [#1303](#1303) + Removed Cygwin pipeline and support [#1283](#1283) ### Interface Changes + Due to internal changes, `ConcreteExecutor::run` will now always throw if the corresponding module for the `ConcreteExecutor` is not build [#1234](#1234) + The constructor of `experimental::distributed::Vector` was changed to only accept local vectors as `std::unique_ptr` [#1284](#1284) + The default parameters for the `solver::MultiGrid` were improved. In particular, the smoother defaults to one iteration of `Ir` with `Jacobi` preconditioner, and the coarse grid solver uses the new direct solver with LU factorization. [#1291](#1291) [#1327](#1327) + The `iteration_complete` event gained a more expressive overload with additional parameters, the old overloads were deprecated. [#1288](#1288) [#1327](#1327) ### Deprecations + Deprecated less expressive `iteration_complete` event. Users are advised to now implement the function `void iteration_complete(const LinOp* solver, const LinOp* b, const LinOp* x, const size_type& it, const LinOp* r, const LinOp* tau, const LinOp* implicit_tau_sq, const array<stopping_status>* status, bool stopped)` [#1288](#1288) ### Added Features + A distributed Schwarz preconditioner. [#1248](#1248) + A GCR solver [#1239](#1239) + Flexible Gmres solver [#1244](#1244) + Enable Gmres solver for distributed matrices and vectors [#1201](#1201) + An example that uses Kokkos to assemble the system matrix [#1216](#1216) + A symbolic LU factorization allowing the `gko::experimental::factorization::Lu` and `gko::experimental::solver::Direct` classes to be used for matrices with non-symmetric sparsity pattern [#1210](#1210) + A numerical Cholesky factorization [#1215](#1215) + Symbolic factorizations in host-side operations are now wrapped in a host-side `Operation` to make their execution visible to loggers. This means that profiling loggers and benchmarks are no longer missing a separate entry for their runtime [#1232](#1232) + Symbolic factorization benchmark [#1302](#1302) + The `ProfilerHook` logger allows annotating the Ginkgo execution (apply, operations, ...) for profiling frameworks like NVTX, ROCTX and TAU. [#1055](#1055) + `ProfilerHook::created_(nested_)summary` allows the generation of a lightweight runtime profile over all Ginkgo functions written to a user-defined stream [#1270](#1270) for both host and device timing functionality [#1313](#1313) + It is now possible to enable host buffers for MPI communications at runtime even if the compile option `GINKGO_FORCE_GPU_AWARE_MPI` is set. [#1228](#1228) + A stencil matrices generator (5-pt, 7-pt, 9-pt, and 27-pt) for benchmarks [#1204](#1204) + Distributed benchmarks (multi-vector blas, SpMV, solver) [#1204](#1204) + Benchmarks for CSR sorting and lookup [#1219](#1219) + A timer for MPI benchmarks that reports the longest time [#1217](#1217) + A `timer_method=min|max|average|median` flag for benchmark timing summary [#1294](#1294) + Support for non-default streams in CUDA and HIP executors [#1236](#1236) + METIS integration for nested dissection reordering [#1296](#1296) + SuiteSparse AMD integration for fillin-reducing reordering [#1328](#1328) + Csr mixed-precision SpMV support [#1319](#1319) + A `with_loggers` function for all `Factory` parameters [#1337](#1337) ### Improvements + Improve naming of kernel operations for loggers [#1277](#1277) + Annotate solver iterations in `ProfilerHook` [#1290](#1290) + Allow using the profiler hooks and inline input strings in benchmarks [#1342](#1342) + Allow passing smart pointers in place of raw pointers to most matrix functions. This means that things like `vec->compute_norm2(x.get())` or `vec->compute_norm2(lend(x))` can be simplified to `vec->compute_norm2(x)` [#1279](#1279) [#1261](#1261) + Catch overflows in prefix sum operations, which makes Ginkgo's operations much less likely to crash. This also improves the performance of the prefix sum kernel [#1303](#1303) + Make the installed GinkgoConfig.cmake file relocatable and follow more best practices [#1325](#1325) ### Fixes + Fix OpenMPI version check [#1200](#1200) + Fix the mpi cxx type binding by c binding [#1306](#1306) + Fix runtime failures for one-sided MPI wrapper functions observed on some OpenMPI versions [#1249](#1249) + Disable thread pinning with GPU executors due to poor performance [#1230](#1230) + Fix hwloc version detection [#1266](#1266) + Fix PAPI detection in non-implicit include directories [#1268](#1268) + Fix PAPI support for newer PAPI versions: [#1321](#1321) + Fix pkg-config file generation for library paths outside prefix [#1271](#1271) + Fix various build failures with ROCm 5.4, CUDA 12, and OneAPI 6 [#1214](#1214), [#1235](#1235), [#1251](#1251) + Fix incorrect read for skew-symmetric MatrixMarket files with explicit diagonal entries [#1272](#1272) + Fix handling of missing diagonal entries in symbolic factorizations [#1263](#1263) + Fix segmentation fault in benchmark matrix construction [#1299](#1299) + Fix the stencil matrix creation for benchmarking [#1305](#1305) + Fix the additional residual check in IR [#1307](#1307) + Fix the cuSPARSE CSR SpMM issue on single strided vector when cuda >= 11.6 [#1322](#1322) [#1331](#1331) + Fix Isai generation for large sparsity powers [#1327](#1327) + Fix Ginkgo compilation and test with NVHPC >= 22.7 [#1331](#1331) + Fix Ginkgo compilation of 32 bit binaries with MSVC [#1349](#1349)

upsj added the 1:ST:ready-for-review This PR is ready for review label Feb 15, 2023

upsj requested review from a team February 15, 2023 09:57

upsj self-assigned this Feb 15, 2023

upsj force-pushed the pointer_param_interface branch from 070edf4 to 8c4cfd8 Compare February 15, 2023 13:14

MarcelKoch self-requested a review February 15, 2023 13:38

MarcelKoch approved these changes Feb 15, 2023

View reviewed changes

MarcelKoch requested review from a team February 15, 2023 13:40

thoasm approved these changes Feb 15, 2023

View reviewed changes

upsj force-pushed the pointer_param_interface branch from 8c4cfd8 to 3abaa66 Compare February 16, 2023 14:15

yhmtsai requested changes Feb 16, 2023

View reviewed changes

MarcelKoch reviewed Feb 17, 2023

View reviewed changes

core/distributed/matrix.cpp Outdated Show resolved Hide resolved

MarcelKoch mentioned this pull request Feb 17, 2023

Accept only unique_ptr for distributed vector constructors #1284

Merged

yhmtsai approved these changes Feb 20, 2023

View reviewed changes

upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Feb 20, 2023

upsj force-pushed the pointer_param_interface branch 2 times, most recently from 8812d22 to f7c0b4d Compare February 20, 2023 15:57

upsj and others added 9 commits February 21, 2023 15:51

add pointer_param

f9b1ae1

make MPI find_package call more consistent

57a5ee7

review updates interface

8158573

- add tests for pointer_param - replace decay::type by decay_t - fix documentation Co-authored-by: Yen-Chen Chen <yen-chen.chen@kit.edu> Co-authored-by: Thomas Grützmacher <thomas.gruetzmacher@kit.edu>

add MPI to README

4f76a58

improve documentation

2893d02

Co-authored-by: Yuhsiang M. Tsai <yhmtsai@gmail.com>

rename pointer_param to ptr_param

1d95a95

rename local_matrix_type to local_matrix_template

9334b77

use pointee where possible

14b7f69

ginkgo-bot force-pushed the pointer_param_interface branch from f7c0b4d to 14b7f69 Compare February 21, 2023 15:51

upsj merged commit d183699 into develop Feb 22, 2023

upsj deleted the pointer_param_interface branch February 22, 2023 09:36

tcojean mentioned this pull request Nov 8, 2023

Release 1.7.0 to master #1451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing smart pointers to functions directly (interface changes only) #1279

Allow passing smart pointers to functions directly (interface changes only) #1279

upsj commented Feb 15, 2023

MarcelKoch left a comment

thoasm left a comment

yhmtsai left a comment •

edited

Loading

yhmtsai Feb 16, 2023

upsj Feb 16, 2023

yhmtsai Feb 16, 2023

upsj Feb 16, 2023

yhmtsai Feb 16, 2023

upsj Feb 16, 2023 •

edited

Loading

yhmtsai Feb 20, 2023

upsj Feb 20, 2023

yhmtsai Feb 20, 2023

upsj Feb 20, 2023

yhmtsai Feb 20, 2023

upsj Feb 20, 2023

MarcelKoch commented Feb 17, 2023

yhmtsai left a comment

yhmtsai Feb 20, 2023

yhmtsai Feb 20, 2023

upsj commented Feb 20, 2023

codecov bot commented Feb 21, 2023 •

edited

Loading

upsj commented Feb 21, 2023

ginkgo-bot commented Feb 21, 2023

sonarqubecloud bot commented Feb 22, 2023

		const device_matrix_data<ValueType, int64>& data,
		pointer_param<const Partition<int64, int64>> partition)

		using ConvertibleTo<SparsityCsr<ValueType, int64>>::convert_to;
		using ConvertibleTo<SparsityCsr<ValueType, int64>>::move_to;

		void row_gather(const array<int32>* gather_indices,
		Dense* row_collection) const;

Allow passing smart pointers to functions directly (interface changes only) #1279

Allow passing smart pointers to functions directly (interface changes only) #1279

Conversation

upsj commented Feb 15, 2023

MarcelKoch left a comment

Choose a reason for hiding this comment

thoasm left a comment

Choose a reason for hiding this comment

yhmtsai left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upsj Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcelKoch commented Feb 17, 2023

yhmtsai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upsj commented Feb 20, 2023

codecov bot commented Feb 21, 2023 • edited Loading

Codecov Report

upsj commented Feb 21, 2023

ginkgo-bot commented Feb 21, 2023

sonarqubecloud bot commented Feb 22, 2023

yhmtsai left a comment •

edited

Loading

upsj Feb 16, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading