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

Make the Csr::automatical strategy work with gko::Executor #425

Closed
thoasm opened this issue Dec 18, 2019 · 4 comments
Closed

Make the Csr::automatical strategy work with gko::Executor #425

thoasm opened this issue Dec 18, 2019 · 4 comments
Labels
is:enhancement An improvement of an existing feature. is:idea Just a thought - if it's good, it could evolve into a proposal. is:interface-breaking This change may break interface compatibility. Can only be done through a major release. is:question This is a question to the developers. mod:core This is related to the core module. type:matrix-format This is related to the Matrix formats

Comments

@thoasm
Copy link
Member

thoasm commented Dec 18, 2019

Currently, we need either a HipExecutor or a CudaExecutor to create the Csr::automatical strategy.
It would be a lot easier to have a constructor that just takes a gko::Executor, so we could use it with any executor.

One way of implementing that is to remove the constructors that take HipExecutor and CudaExecutor and replace it with one that just takes Executor, which uses dynamic_cast to check for CUDA and HIP executors, while also working for different executors.

However, this might break interface, so we have to discuss on that.

@thoasm thoasm added is:enhancement An improvement of an existing feature. mod:core This is related to the core module. is:idea Just a thought - if it's good, it could evolve into a proposal. type:matrix-format This is related to the Matrix formats is:question This is a question to the developers. labels Dec 18, 2019
@upsj
Copy link
Member

upsj commented Dec 18, 2019

This is also slightly related to the "double-destroy" segfaults we had with #418, since the automatical strategy is default-constructed with a CudaExecutor

@tcojean
Copy link
Member

tcojean commented Dec 18, 2019

The default CudaExecutor was here before, because automatical in the previous releases was purely a Cuda strategy. Changing this would definitely break interface. We can consider this a bugfix maybe and then justify that change.

On the other hand, the change proposed here should be fine, we would have the same constructor with the same parameters which should do the same thing as currently given the same argument.

@thoasm thoasm self-assigned this Dec 18, 2019
@thoasm thoasm removed their assignment Jul 3, 2020
@thoasm thoasm added the is:interface-breaking This change may break interface compatibility. Can only be done through a major release. label Jul 3, 2020
@upsj upsj added this to the Ginkgo 2.0.0 milestone May 7, 2021
@tcojean
Copy link
Member

tcojean commented Feb 10, 2022

#969 implemented a non-interface breaking way of achieving the same result. But Csr::automatical and strategies in general still need to be fixed.

@upsj upsj removed this from the Ginkgo 2.0.0 milestone Nov 2, 2022
@upsj
Copy link
Member

upsj commented Jan 26, 2023

Sounds like the remaining work is covered by #320, I think this can be closed

@upsj upsj closed this as completed Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:enhancement An improvement of an existing feature. is:idea Just a thought - if it's good, it could evolve into a proposal. is:interface-breaking This change may break interface compatibility. Can only be done through a major release. is:question This is a question to the developers. mod:core This is related to the core module. type:matrix-format This is related to the Matrix formats
Projects
None yet
Development

No branches or pull requests

3 participants