-
Notifications
You must be signed in to change notification settings - Fork 89
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
Make criterion factory parameters work with CUDA #586
Conversation
2aa5ba9
to
e79a20d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@yhmtsai Sure, let me know if it works. |
Is it possible to still have the recursive version for the host compiler, and the fix you implemented for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
e79a20d
to
b9a1709
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM though for now somethings are unclear for these parameters which can be both scalar and vector in BJ
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add an example with GKO_FACTORY_PARAMETER_VECTOR
to also showcase this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
* 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, but don't give both the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Nice solution to leave the old GKO_FACTORY_PARAMETER
and mark it as deprecated. That way, we don't change an existing interface.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, but don't give both the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
some confusion on pair example
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <thomas.gruetzmacher@kit.edu> Co-authored-by: Terry Cojean <terry.cojean@kit.edu> Co-authored-by: Yuhsiang M. Tsai <yhmtsai@gmail.com>
b9a1709
to
3ade128
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage 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. |
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
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 byGKO_FACTORY_PARAMETER
in C++ compilers.It also adds a NotImplemented exception for nvcc using
GKO_FACTORY_PARAMETER
.Fixes #491