search

intel / llvm

Intel staging area for llvm.org contribution. Home for Intel LLVM-based projects.
Other
1.25k stars 738 forks source link

"discard queue event" should still allow event to be used #12388

Open TApplencourt opened 10 months ago

TApplencourt commented 10 months ago

Is your feature request related to a problem? Please describe

Discard event ( sycl_ext_oneapi_discard_queue_events is really useful.

Sadly, it restricts any usage of the event. Meaning that, I cannot create a queue with this property and then pass it to oneMKL for example.

Pseudocode

  sycl::property_list props{ext::oneapi::property::queue::discard_events{},
                            property::queue::in_order{}};
  sycl::queue Q( props );
  oneMKL::dgem(Q, ...);  // This will fail, as dgem use event internally
  Q.wait();

Describe the solution you would like

It's easy. Just implement discard_event like HipSYCL is doing it. Meaning when using the event (either for .wait or depend_on) just submit a Q.wait().

This will solve the composability issue.

Additional context

See more info here: https://github.com/KhronosGroup/SYCL-Docs/issues/404).

(@abagusetty)

0x12CC commented 5 months ago

@TApplencourt, thanks for this suggestion. Our current implementation of .wait() for invalid events throws a SYCL exception: what(): wait method cannot be used for a discarded event.

@Pennycook, do you think this should require a new extension? It doesn't seem like the requested change is compatible with the sycl_ext_oneapi_discard_queue_events specification.

Pennycook commented 4 months ago

@0x12CC : I'm not sure. Such a change would permit something that previously wasn't allowed, so I don't think it would count as a "breaking change". But I'm reluctant to change sycl_ext_oneapi_discard_queue_events at this stage, primarily because I don't think people should be writing new code that uses it. sycl_ext_oneapi_enqueue_functions is our preferred solution to enqueuing commands without events. @gmlueck, do you agree?

gmlueck commented 4 months ago

Yes, I was thinking that we would probably deprecate and then remove "sycl_ext_oneapi_discard_queue_events" now that we have "sycl_ext_oneapi_enqueue_functions"

0x12CC commented 4 months ago

@TApplencourt, the sycl_ext_oneapi_enqueue_functions extension allows you to enqueue work without creating an event object. It doesn't require a special queue so it's compatible with libraries like oneMKL. Is this acceptable for what you're trying to do?

TApplencourt commented 4 months ago

The barrier of entry is not the same. For big applications, sycl_ext_oneapi_discard_queue_events is a one-line change, porting all their submissions to sycl_ext_oneapi_enqueue_functions is a monthly effort.

gmlueck commented 4 months ago

The barrier of entry is not the same. For big applications, sycl_ext_oneapi_discard_queue_events is a one-line change, porting all their submissions to sycl_ext_oneapi_enqueue_functions is a monthly effort.

Is it really that big of a change? Porting to sycl_ext_oneapi_enqueue_functions is still a localized change in the code, affecting just the few lines that submit a kernel. Th ere is no need to change the entire application, either. You can just change one kernel submission if you want, leaving the remaining submissions to use the old API.

TApplencourt commented 4 months ago

True, It's just that people have more kernels (100+) than queues. The implementation cost seems low to me and has a high reward, but that makes sense if you prefer to wait for the "official" fix. @abagusetty, what do you think? You where the one looking for this :)

abagusetty commented 4 months ago

@TApplencourt That should work as well, Thanks will give it a try.

github-actions[bot] commented 2 months ago

Hi! There have been no updates for at least the last 60 days, though the issue has assignee(s).

@0x12CC, could you please take one of the following actions:

Thanks!

github-actions[bot] commented 1 week ago

Hi! There have been no updates for at least the last 60 days, though the issue has assignee(s).

@0x12CC, could you please take one of the following actions:

Thanks!