Make criterion factory parameters work with CUDA #586

upsj · 2020-07-07T16:47:30Z

This PR adds new "overloads" GKO_FACTORY_PARAMETER_SCALAR/_VECTOR for types which can be initialized with a single value/multiple values which are aliased to by GKO_FACTORY_PARAMETER in C++ compilers.
It also adds a NotImplemented exception for nvcc using GKO_FACTORY_PARAMETER.

Fixes #491

tcojean

LGTM

include/ginkgo/core/base/lin_op.hpp

codecov · 2020-07-08T11:06:11Z

Codecov Report

Merging #586 into develop will increase coverage by 0.15%.
The diff coverage is 96.49%.

@@             Coverage Diff             @@
##           develop     #586      +/-   ##
===========================================
+ Coverage    92.85%   93.01%   +0.15%     
===========================================
  Files          303      296       -7     
  Lines        21105    20656     -449     
===========================================
- Hits         19598    19214     -384     
+ Misses        1507     1442      -65

Impacted Files	Coverage Δ
include/ginkgo/core/base/lin_op.hpp	`97.64% <ø> (ø)`
include/ginkgo/core/factorization/ilu.hpp	`0.00% <ø> (ø)`
include/ginkgo/core/preconditioner/ilu.hpp	`96.93% <66.66%> (ø)`
include/ginkgo/core/factorization/par_ict.hpp	`94.44% <80.00%> (ø)`
core/test/base/lin_op.cpp	`92.40% <100.00%> (ø)`
include/ginkgo/core/factorization/par_ilu.hpp	`100.00% <100.00%> (ø)`
include/ginkgo/core/factorization/par_ilut.hpp	`100.00% <100.00%> (ø)`
include/ginkgo/core/preconditioner/isai.hpp	`100.00% <100.00%> (ø)`
include/ginkgo/core/preconditioner/jacobi.hpp	`92.00% <100.00%> (ø)`
include/ginkgo/core/solver/bicg.hpp	`100.00% <100.00%> (ø)`
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb62766...3ade128. Read the comment docs.

yhmtsai

LGTM.
GKO_FACTORY_PARAMETER_VECTOR will give the empty vector not {nullptr}, right?
Also, if do not need to merge this pr with high priority, I would like to try it on multigrid before it's merged.

include/ginkgo/core/base/lin_op.hpp

upsj · 2020-07-08T21:32:24Z

@yhmtsai Sure, let me know if it works.

thoasm · 2020-07-09T08:40:05Z

Is it possible to still have the recursive version for the host compiler, and the fix you implemented for nvcc? That way, it is no longer interface breaking.
The problem is that we now can have up to 5 (not counting custom) stopping criterion pushed to master, meaning we would need to have at least 5 overloads (with 1-5 criterions) to not break potentially existing code.

thoasm

This currently breaks the interface too much to be considered a fix.
It would be nice to have the old implementation for host compiled code, and a better implementation for nvcc compiled code that works for a certain number of stopping criterions (might be good to throw if it would no longer work instead of silently not setting the values).

include/ginkgo/core/base/lin_op.hpp

stale

tcojean

LGTM though for now somethings are unclear for these parameters which can be both scalar and vector in BJ

tcojean · 2020-08-06T14:17:16Z

include/ginkgo/core/base/lin_op.hpp

@@ -753,7 +753,7 @@ public: \
 * GKO_ENABLE_LIN_OP_FACTORY(MyLinOp, my_parameters, Factory) {
 * // a factory parameter named "my_value", of type int and default
 * // value of 5
- * int GKO_FACTORY_PARAMETER(my_value, 5);
+ * int GKO_FACTORY_PARAMETER_SCALAR(my_value, 5);


Maybe add an example with GKO_FACTORY_PARAMETER_VECTOR to also showcase this one?

Good point!

Suggested change

* int GKO_FACTORY_PARAMETER_SCALAR(my_value, 5);

* int GKO_FACTORY_PARAMETER_SCALAR(my_value, 5);

* // a factory parameter named `my_pair` of type `std::pair<int,int>`

* // and default value {5, 5}

* std::pair<int, int> GKO_FACTORY_PARAMETER_VECTOR(my_pair, 5, 5);

good idea, but don't give both the same name.

I am confused with std::pair example.
I feel std::pair is a scalar containing two elements like complex type.
maybe like std::pair<int, int> GKO_FACTORY_PARAMETER_SCALAR(my_value, make_pair(5, 5))?

The difference between _SCALAR and _VECTOR comes from how cudafe++ translates the files. I'm not entirely sure about the formal criteria, but for int and other primitive types, the parameter pack expansion gets translated incorrectly (Into a C-style cast instead of a constructor call), while for composite types like std::pair, std::vector, ... it correctly invokes the constructor. The semantics of this are a bit fuzzy, though, that is true. You could also use SCALAR, but then you would have to use make_pair or braces { ... } for the default argument or with_my_value.

include/ginkgo/core/base/lin_op.hpp

include/ginkgo/core/preconditioner/jacobi.hpp

include/ginkgo/core/base/lin_op.hpp

thoasm

LGTM!
Nice solution to leave the old GKO_FACTORY_PARAMETER and mark it as deprecated. That way, we don't change an existing interface.

cuda/test/base/lin_op.cu

hip/test/base/lin_op.hip.cpp

thoasm · 2020-08-12T14:19:22Z

include/ginkgo/core/base/lin_op.hpp

@@ -753,7 +753,7 @@ public: \
 * GKO_ENABLE_LIN_OP_FACTORY(MyLinOp, my_parameters, Factory) {
 * // a factory parameter named "my_value", of type int and default
 * // value of 5
- * int GKO_FACTORY_PARAMETER(my_value, 5);
+ * int GKO_FACTORY_PARAMETER_SCALAR(my_value, 5);


good idea, but don't give both the same name.

yhmtsai

LGTM.
some confusion on pair example

include/ginkgo/core/base/lin_op.hpp

yhmtsai · 2020-08-13T11:06:10Z

include/ginkgo/core/base/lin_op.hpp

@@ -753,7 +753,7 @@ public: \
 * GKO_ENABLE_LIN_OP_FACTORY(MyLinOp, my_parameters, Factory) {
 * // a factory parameter named "my_value", of type int and default
 * // value of 5
- * int GKO_FACTORY_PARAMETER(my_value, 5);
+ * int GKO_FACTORY_PARAMETER_SCALAR(my_value, 5);


I am confused with std::pair example.
I feel std::pair is a scalar containing two elements like complex type.
maybe like std::pair<int, int> GKO_FACTORY_PARAMETER_SCALAR(my_value, make_pair(5, 5))?

nvcc has issues with parameter pack expansion assigned to primitive-type variables. This can be worked around with a backward-compatible interface by adding _SCALAR and _VECTOR "overloads" and deprecating the plain GKO_FACTORY_PARAMETER

Co-authored-by: Thomas Grützmacher <[email protected]> Co-authored-by: Terry Cojean <[email protected]> Co-authored-by: Yuhsiang M. Tsai <[email protected]>

sonarcloud · 2020-08-19T17:44:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

The version of Java (1.8.0_121) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

Release 1.3.0 of Ginkgo. The Ginkgo team is proud to announce the new minor release of Ginkgo version 1.3.0. This release brings CUDA 11 support, changes the default C++ standard to be C++14 instead of C++11, adds a new Diagonal matrix format and capacity for diagonal extraction, significantly improves the CMake configuration output format, adds the Ginkgo paper which got accepted into the Journal of Open Source Software (JOSS), and fixes multiple issues. Supported systems and requirements: + For all platforms, cmake 3.9+ + Linux and MacOS + gcc: 5.3+, 6.3+, 7.3+, all versions after 8.1+ + clang: 3.9+ + Intel compiler: 2017+ + Apple LLVM: 8.0+ + CUDA module: CUDA 9.0+ + HIP module: ROCm 2.8+ + Windows + MinGW and Cygwin: gcc 5.3+, 6.3+, 7.3+, all versions after 8.1+ + Microsoft Visual Studio: VS 2017 15.7+ + CUDA module: CUDA 9.0+, Microsoft Visual Studio + OpenMP module: MinGW or Cygwin. The current known issues can be found in the [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues). Additions: + Add paper for Journal of Open Source Software (JOSS). [#479](#479) + Add a DiagonalExtractable interface. [#563](#563) + Add a new diagonal Matrix Format. [#580](#580) + Add Cuda11 support. [#603](#603) + Add information output after CMake configuration. [#610](#610) + Add a new preconditioner export example. [#595](#595) + Add a new cuda-memcheck CI job. [#592](#592) Changes: + Use unified memory in CUDA debug builds. [#621](#621) + Improve `BENCHMARKING.md` with more detailed info. [#619](#619) + Use C++14 standard instead of C++11. [#611](#611) + Update the Ampere sm information and CudaArchitectureSelector. [#588](#588) Fixes: + Fix documentation warnings and errors. [#624](#624) + Fix warnings for diagonal matrix format. [#622](#622) + Fix criterion factory parameters in CUDA. [#586](#586) + Fix the norm-type in the examples. [#612](#612) + Fix the WAW race in OpenMP is_sorted_by_column_index. [#617](#617) + Fix the example's exec_map by creating the executor only if requested. [#602](#602) + Fix some CMake warnings. [#614](#614) + Fix Windows building documentation. [#601](#601) + Warn when CXX and CUDA host compiler do not match. [#607](#607) + Fix reduce_add, prefix_sum, and doc-build. [#593](#593) + Fix find_library(cublas) issue on machines installing multiple cuda. [#591](#591) + Fix allocator in sellp read. [#589](#589) + Fix the CAS with HIP and NVIDIA backends. [#585](#585) Deletions: + Remove unused preconditioner parameter in LowerTrs. [#587](#587) Related PR: #625

The Ginkgo team is proud to announce the new minor release of Ginkgo version 1.3.0. This release brings CUDA 11 support, changes the default C++ standard to be C++14 instead of C++11, adds a new Diagonal matrix format and capacity for diagonal extraction, significantly improves the CMake configuration output format, adds the Ginkgo paper which got accepted into the Journal of Open Source Software (JOSS), and fixes multiple issues. Supported systems and requirements: + For all platforms, cmake 3.9+ + Linux and MacOS + gcc: 5.3+, 6.3+, 7.3+, all versions after 8.1+ + clang: 3.9+ + Intel compiler: 2017+ + Apple LLVM: 8.0+ + CUDA module: CUDA 9.0+ + HIP module: ROCm 2.8+ + Windows + MinGW and Cygwin: gcc 5.3+, 6.3+, 7.3+, all versions after 8.1+ + Microsoft Visual Studio: VS 2017 15.7+ + CUDA module: CUDA 9.0+, Microsoft Visual Studio + OpenMP module: MinGW or Cygwin. The current known issues can be found in the [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues). Additions: + Add paper for Journal of Open Source Software (JOSS). [#479](#479) + Add a DiagonalExtractable interface. [#563](#563) + Add a new diagonal Matrix Format. [#580](#580) + Add Cuda11 support. [#603](#603) + Add information output after CMake configuration. [#610](#610) + Add a new preconditioner export example. [#595](#595) + Add a new cuda-memcheck CI job. [#592](#592) Changes: + Use unified memory in CUDA debug builds. [#621](#621) + Improve `BENCHMARKING.md` with more detailed info. [#619](#619) + Use C++14 standard instead of C++11. [#611](#611) + Update the Ampere sm information and CudaArchitectureSelector. [#588](#588) Fixes: + Fix documentation warnings and errors. [#624](#624) + Fix warnings for diagonal matrix format. [#622](#622) + Fix criterion factory parameters in CUDA. [#586](#586) + Fix the norm-type in the examples. [#612](#612) + Fix the WAW race in OpenMP is_sorted_by_column_index. [#617](#617) + Fix the example's exec_map by creating the executor only if requested. [#602](#602) + Fix some CMake warnings. [#614](#614) + Fix Windows building documentation. [#601](#601) + Warn when CXX and CUDA host compiler do not match. [#607](#607) + Fix reduce_add, prefix_sum, and doc-build. [#593](#593) + Fix find_library(cublas) issue on machines installing multiple cuda. [#591](#591) + Fix allocator in sellp read. [#589](#589) + Fix the CAS with HIP and NVIDIA backends. [#585](#585) Deletions: + Remove unused preconditioner parameter in LowerTrs. [#587](#587) Related PR: #627

upsj added is:bug Something looks wrong. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. labels Jul 7, 2020

upsj self-assigned this Jul 7, 2020

upsj force-pushed the fix_cuda_factory_parameters branch 2 times, most recently from 2aa5ba9 to e79a20d Compare July 8, 2020 07:25

tcojean previously approved these changes Jul 8, 2020

View reviewed changes

upsj commented Jul 8, 2020

View reviewed changes

include/ginkgo/core/base/lin_op.hpp Outdated Show resolved Hide resolved

upsj requested review from yhmtsai, thoasm, fritzgoebel, hartwiganzt and pratikvn July 8, 2020 13:43

upsj added the 1:ST:ready-for-review This PR is ready for review label Jul 8, 2020

pratikvn previously approved these changes Jul 8, 2020

View reviewed changes

yhmtsai previously approved these changes Jul 8, 2020

View reviewed changes

include/ginkgo/core/base/lin_op.hpp Outdated Show resolved Hide resolved

thoasm previously requested changes Jul 9, 2020

View reviewed changes

include/ginkgo/core/base/lin_op.hpp Outdated Show resolved Hide resolved

upsj added 1:ST:WIP This PR is a work in progress. Not ready for review. and removed 1:ST:ready-for-review This PR is ready for review labels Jul 13, 2020

tcojean self-requested a review July 29, 2020 15:58

upsj force-pushed the fix_cuda_factory_parameters branch from e79a20d to b9a1709 Compare August 6, 2020 13:26

upsj requested review from yhmtsai and thoasm August 6, 2020 13:30

upsj requested a review from pratikvn August 6, 2020 13:30

tcojean mentioned this pull request Aug 6, 2020

ERROR when build the ginkgo example with MFEM and CUDA #616

Closed

upsj added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Aug 6, 2020

tcojean approved these changes Aug 6, 2020

View reviewed changes

upsj commented Aug 6, 2020

View reviewed changes

include/ginkgo/core/base/lin_op.hpp Outdated Show resolved Hide resolved

thoasm approved these changes Aug 12, 2020

View reviewed changes

tcojean 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 Aug 13, 2020

yhmtsai approved these changes Aug 13, 2020

View reviewed changes

upsj and others added 2 commits August 13, 2020 13:18

fix GKO_FACTORY_PARAMETER for CUDA

4c4e44e

nvcc has issues with parameter pack expansion assigned to primitive-type variables. This can be worked around with a backward-compatible interface by adding _SCALAR and _VECTOR "overloads" and deprecating the plain GKO_FACTORY_PARAMETER

review updates

3ade128

Co-authored-by: Thomas Grützmacher <[email protected]> Co-authored-by: Terry Cojean <[email protected]> Co-authored-by: Yuhsiang M. Tsai <[email protected]>

upsj force-pushed the fix_cuda_factory_parameters branch from b9a1709 to 3ade128 Compare August 13, 2020 12:25

upsj merged commit ba147c0 into develop Aug 13, 2020

upsj deleted the fix_cuda_factory_parameters branch August 13, 2020 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make criterion factory parameters work with CUDA #586

Make criterion factory parameters work with CUDA #586

upsj commented Jul 7, 2020 •

edited

Loading

tcojean left a comment

codecov bot commented Jul 8, 2020 •

edited

Loading

yhmtsai left a comment

upsj commented Jul 8, 2020

thoasm commented Jul 9, 2020

thoasm left a comment

tcojean left a comment

tcojean Aug 6, 2020

upsj Aug 6, 2020 •

edited

Loading

thoasm Aug 12, 2020

yhmtsai Aug 13, 2020

upsj Aug 13, 2020

thoasm left a comment

thoasm Aug 12, 2020

yhmtsai left a comment

yhmtsai Aug 13, 2020

sonarcloud bot commented Aug 19, 2020

- * int GKO_FACTORY_PARAMETER_SCALAR(my_value, 5);
+ * int GKO_FACTORY_PARAMETER_SCALAR(my_value, 5);
+ * // a factory parameter named `my_pair` of type `std::pair<int,int>`
+ * // and default value {5, 5}
+ * std::pair<int, int> GKO_FACTORY_PARAMETER_VECTOR(my_pair, 5, 5);

Make criterion factory parameters work with CUDA #586

Make criterion factory parameters work with CUDA #586

Conversation

upsj commented Jul 7, 2020 • edited Loading

tcojean left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 8, 2020 • edited Loading

Codecov Report

yhmtsai left a comment

Choose a reason for hiding this comment

upsj commented Jul 8, 2020

thoasm commented Jul 9, 2020

thoasm left a comment

Choose a reason for hiding this comment

tcojean left a comment

Choose a reason for hiding this comment

tcojean Aug 6, 2020

Choose a reason for hiding this comment

upsj Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

thoasm Aug 12, 2020

Choose a reason for hiding this comment

yhmtsai Aug 13, 2020

Choose a reason for hiding this comment

upsj Aug 13, 2020

Choose a reason for hiding this comment

thoasm left a comment

Choose a reason for hiding this comment

thoasm Aug 12, 2020

Choose a reason for hiding this comment

yhmtsai left a comment

Choose a reason for hiding this comment

yhmtsai Aug 13, 2020

Choose a reason for hiding this comment

sonarcloud bot commented Aug 19, 2020

upsj commented Jul 7, 2020 •

edited

Loading

codecov bot commented Jul 8, 2020 •

edited

Loading

upsj Aug 6, 2020 •

edited

Loading