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

cgtblas: sycl backend does not handle nullptr case #216

Open
bd4 opened this issue Nov 3, 2022 · 3 comments
Open

cgtblas: sycl backend does not handle nullptr case #216

bd4 opened this issue Nov 3, 2022 · 3 comments

Comments

@bd4
Copy link
Contributor

bd4 commented Nov 3, 2022

Existing use of gpublas_set_stream in GENE passes nullptr to reset to default stream behavior. The stream refactor broke this for SYCL backend, where that has to be special cased to handle. For CUDA and HIP backends, the underlying type is a pointer and nullptr is the low level value that means resetting.

The easiest fix would be to add the special casing in SYCL specialization, but it's still a fragile design for the C interface, exposing this low level thing that just happens to work without special casing on some backends. We could have a gpublas_set_stream_default() as part of the interface. Also, GENE is not currently benefiting from using a separate stream for the solve, so we could remove these while we decide on a design.

@germasch
Copy link
Contributor

germasch commented Nov 3, 2022

So I think there's been some subtlety in the type used for sycl, anyway, in that a reference type (queue&) has been used, if I even understood things correctly. It seems to me that sycl could be made to behave more cuda/hip like by having some

struct sycl_stream_t
{
   sycl::queue *q;
}

And that could hopefully be used to eliminate those subtle differences at the interface level?

@bd4
Copy link
Contributor Author

bd4 commented Nov 4, 2022

The queue is the gateway to all operations in SYCL, so it can't be null - something needs to intercept the attempt to set a null queue and replace it with the default for that device, or the interface needs to change so setting a null "stream" is not alowed, and resetting to the default is done another way.

@germasch
Copy link
Contributor

germasch commented Nov 4, 2022

Right, the queue can't be null, but a pointer to it can be. So if that's what we're passing from Fortran, we can still use C_NULL_PTR. Currently, we directly use queue&, which never can be officially NULL (though in practice if one passes is from Fortran, with Fortran thinking it's a pointer, it will be null. On the C side, one basically has to go, somewhere,

if (queue_pointer) return *queue; else return default queue;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants