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

Remove unused vars in sycl resource #84

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidbeckingsale
Copy link
Member

No description provided.

@trws
Copy link
Member

trws commented Sep 9, 2021

This is freaking me out right now, because without the queue I don't think the event actually syncs anything. It's defined to create an already ready event. More oddly, there doesn't seem to be a sycl API for creating an event independently from running work on the device, looks like we'll have to pull the OpenCL queue out of the sycl queue and use clEnqueueMarkerWithWaitList to create an event that actually works.

That and add more tests, because there's no way this can possibly be working correctly.

@davidbeckingsale
Copy link
Member Author

😂 yeah I just saw the unused stuff when I was building Umpire - we are not actually using any of the sycl resources at the moment. I think we probably need to run this by @homerdin ?

@trws
Copy link
Member

trws commented Sep 9, 2021

Definitely, and need to get some dependency tests so we can actually verify all this stuff, I'll think about it. Maybe we can get something together that will be portable with a polymorphic resource? Not sure, but it would help.

@homerdin
Copy link
Contributor

@trws Yeah, this event doesn't work currently (was focused on the resource). I think the main reason for the limited sycl event interface is from the out-of-order queue default. Relying on the OpenCL queue could create an issue when using level_zero.

If you expect camp events to only be used with the camp resources which have in-order queues we might be able to submit a do-nothing task to the queue and use that event (need to test). Otherwise creating an event to wait on everything in currently in the queue of an out-of-order queue might be non-deterministic.

I'm not familiar with all your use cases, would it be useful to have the camp sycl event store a set of dependencies that it needs to wait for? I'm not sure if having an option to build a find grained DAG would be useful.

@trws
Copy link
Member

trws commented Sep 13, 2021 via email

@trws
Copy link
Member

trws commented Sep 14, 2021

@xtian-github, @RaviNarayanaswamy, sorry to bug you in an odd way for this, but it looks like we have a gap in the sycl api that concerns me a little bit. Is there a way to get an event handle that will wait on all tasks enqueued on a queue through dpc++? I know we can do this through OpenCL, but if for whatever reason that doesn't work on level zero or similar I don't see a way to do this through the actual sycl APIs. I'm asking on camp because this is where I ran into it, but this would be a problem for correct implementation of the OpenMP interop construct as well.

@xtian-github
Copy link

@trws do you mean to get native L0 event handle in DPC++? If so, the answer is yes. It is a feature we have just added.

@trws
Copy link
Member

trws commented Sep 14, 2021

Not specifically, though that might work @xtian-github. The issue is that here in camp, and in interop for that matter, the logical model is that there is a way to wait on all the work that has been put onto a queue (or in the camp setup a resource) before a certain point. Basically what should go here to generate an event that will wait on everything previously submitted to qu.

As far as I can tell, the sycl::queue offers no way to create an event that does that. In OpenCL land I can create a marker event on the cl_queue object easily enough, and that works fine, but since the RAJA and camp backends are written in sycl and dpc++ doesn't seem to want to stick to OpenCL I'm not sure if that's a safe thing to do. If we have to do the OpenCL way in general and on DPC++ use L0 we could do that but I was hoping we could find a way to do this in the higher level APIs somehow, or at least keep it to one low-level API.

@xtian-github
Copy link

are you asking something like q.wait_all_events ?

@trws
Copy link
Member

trws commented Sep 15, 2021

It looks like there's an extension proposal for this, that I somehow missed. We need q.submit_barrier() (proposal) to make this interface work.

To summarize it, we need to be able to get an object that we can later use to synchronize with all things that were enqueued before the object was created. The wait methods don't help us.

If this is in DPC++, which it seems to be, then I think the solution is we use submit_barrier on DPC++ and OpenCL's event marker elsewhere. That won't help us on hipsycl AFAICT, but I don't see any way to do it in current standard sycl.

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.

4 participants