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

SYCL Resource - In order queue #63

Merged
merged 5 commits into from
Jul 23, 2021
Merged

SYCL Resource - In order queue #63

merged 5 commits into from
Jul 23, 2021

Conversation

homerdin
Copy link
Contributor

Changing SYCL resources to use in order queue property to ensure expected functionality.

@trws
Copy link
Member

trws commented Jun 22, 2021

I was reminded to revisit this in the meeting this morning. @homerdin, I'm good with this conceptually, but doesn't creating them this way create them all with separate contexts? I would think we need the resource implementation to have or take a context (or get one from the first queue perhaps) and use it to create the others to make them compatible right?

@homerdin
Copy link
Contributor Author

@trws I've updated the sycl resource to use a single context and optionally take a context on construction.

@trws
Copy link
Member

trws commented Jun 29, 2021

Thanks @homerdin, this looks good for the default case. What will it do with a specified context though? If I'm interpreting it right, since the static is only initialized once, I think the list of queues will get initialized with whatever context is used the first time a queue is requested and then all subsequent calls will ignore the context and give one of the queues created at the beginning. Is that right?

@homerdin
Copy link
Contributor Author

homerdin commented Jul 2, 2021

@trws Yeah, the subsequent calls will give a queue from the original context. We can add some dynamic memory if they want to build a resource from a second context. I'm not sure if any application will building multiple contexts. At a minimum I should add some messaging if a second context gets passed in. Let me know and I can add some functionality for working with multiple contexts.

@trws
Copy link
Member

trws commented Jul 2, 2021 via email

@homerdin
Copy link
Contributor Author

homerdin commented Jul 10, 2021

@trws I took a pass at adding support for different contexts, let me know what you think.

Currently the logic is to use the context when given. When no context is given, use the previous one or a private one(when the user never creates one).

This would require that if the user is creating multiple resources on different threads with thread private contexts, they would need to be explicit each time and not rely on the previous context passed.

@trws
Copy link
Member

trws commented Jul 21, 2021

That works for me. I just went in and formatted it to match style, and will rebase it shortly.

@trws
Copy link
Member

trws commented Jul 21, 2021

@homerdin, this is rebased and formatted. Would you take a look and maybe poke at it before I merge to make sure something didn't break in the process?

@homerdin
Copy link
Contributor Author

@trws I pulled down the changed and tested them. Everything looks good.

@trws
Copy link
Member

trws commented Jul 23, 2021

Excellent, thanks @homerdin!

@trws trws merged commit b1eac9e into LLNL:master Jul 23, 2021
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

Successfully merging this pull request may close these issues.

None yet

2 participants