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.

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

Allow passing smart pointers to functions directly #1261

Merged
merged 6 commits into from
Feb 22, 2023
Merged

Conversation

upsj
Copy link
Member

@upsj upsj commented Jan 23, 2023

This adds a pointer_param class which can be converted to from all types of pointers, as a replacement for raw pointer parameters.

TODO:

Closes #179

@ginkgo-bot ginkgo-bot added mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:dpcpp This is related to the DPC++ module. mod:hip This is related to the HIP module. mod:reference This is related to the reference module. reg:benchmarking This is related to benchmarking. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:matrix-format This is related to the Matrix formats type:multigrid This is related to multigrid type:preconditioner This is related to the preconditioners type:solver This is related to the solvers labels Jan 23, 2023
@upsj upsj force-pushed the pointer_param branch 2 times, most recently from facf6dc to baaff20 Compare February 1, 2023 11:07
@upsj upsj marked this pull request as ready for review February 1, 2023 11:07
@upsj upsj requested a review from a team February 1, 2023 11:08
@upsj upsj added 1:ST:ready-for-review This PR is ready for review 1:ST:run-full-test labels Feb 1, 2023
@thoasm thoasm self-requested a review February 1, 2023 13:06
@MarcelKoch MarcelKoch self-requested a review February 2, 2023 09:03
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

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

Documentation is missing and I would like to see the functions write and write_binary have changed template parameter.
The reset looks good and I like the new interface without most of the gko::lend.

CMakeLists.txt Outdated Show resolved Hide resolved
include/ginkgo/core/base/utils_helper.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/polymorphic_object.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/polymorphic_object.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/polymorphic_object.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/mtx_io.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/matrix/csr.hpp Outdated Show resolved Hide resolved
test/factorization/ic_kernels.cpp Outdated Show resolved Hide resolved
test/factorization/ilu_kernels.cpp Outdated Show resolved Hide resolved
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

I like the changes, I think they should be helpful for users to write more concise code. I mostly went over the changes in our public header, and for the rest searching for lend doesn't return any results, so I think it should be fine.

Perhaps this would be a good place to deprecate PolymorphicObject::copy_from(std::unique_ptr).
After this gets merged, we should add a task to our Ginkgo 2.0 list about removing virtual functions with bare pointer parameters if a pointer_param overload exists.

include/ginkgo/core/base/utils_helper.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/lin_op.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/polymorphic_object.hpp Outdated Show resolved Hide resolved
cmake/GinkgoConfig.cmake.in Outdated Show resolved Hide resolved
cmake/create_test.cmake Outdated Show resolved Hide resolved
@upsj
Copy link
Member Author

upsj commented Feb 4, 2023

format-rebase!

@ginkgo-bot
Copy link
Member

Formatting rebase introduced changes, see Artifacts here to review them

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Base: 91.24% // Head: 91.37% // Increases project coverage by +0.13% 🎉

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

❗ Current head df52074 differs from pull request most recent head ddd541a. Consider uploading reports for the commit ddd541a to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1261      +/-   ##
===========================================
+ Coverage    91.24%   91.37%   +0.13%     
===========================================
  Files          565      565              
  Lines        48026    47823     -203     
===========================================
- Hits         43822    43700     -122     
+ Misses        4204     4123      -81     
Impacted Files Coverage Δ
core/factorization/ic.cpp 96.87% <ø> (ø)
core/factorization/ilu.cpp 100.00% <ø> (ø)
core/factorization/par_ic.cpp 100.00% <ø> (ø)
core/log/convergence.cpp 100.00% <ø> (ø)
core/matrix/fft.cpp 74.50% <0.00%> (ø)
core/solver/bicgstab.cpp 90.26% <ø> (ø)
core/solver/cb_gmres.cpp 25.97% <0.00%> (-70.53%) ⬇️
core/solver/cg.cpp 87.34% <ø> (ø)
core/solver/cgs.cpp 89.36% <ø> (ø)
core/solver/fcg.cpp 88.09% <ø> (ø)
... and 243 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.

Base automatically changed from pointer_param_interface to develop February 22, 2023 09:36
@upsj
Copy link
Member Author

upsj commented Feb 22, 2023

format-rebase!

@ginkgo-bot
Copy link
Member

Formatting rebase introduced changes, see Artifacts here to review them

@ginkgo-bot
Copy link
Member

Note: This PR changes the Ginkgo ABI:

Functions changes summary: 216 Removed, 0 Changed, 218 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

For details check the full ABI diff under Artifacts here

@upsj 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 1:ST:run-full-test labels Feb 22, 2023
@upsj upsj merged commit ed32fed into develop Feb 22, 2023
@upsj upsj deleted the pointer_param branch February 22, 2023 19:35
@sonarcloud
Copy link

sonarcloud bot commented Feb 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 21 Code Smells

74.8% 74.8% Coverage
4.6% 4.6% Duplication

tcojean added a commit that referenced this pull request Jun 16, 2023
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:dpcpp This is related to the DPC++ module. mod:hip This is related to the HIP module. mod:reference This is related to the reference module. reg:benchmarking This is related to benchmarking. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:matrix-format This is related to the Matrix formats type:multigrid This is related to multigrid type:preconditioner This is related to the preconditioners type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alternatives to raw pointers
8 participants