Add clarification to event wait wording

[AerialMantis]

Motivation

It was discovered that the current wording for event::wait_and_throw was ambiguous.

Table 34

Wait for the event and the command associated with it to complete.

Any unconsumed async-error from any context that the event was waiting on executions from will be passed to the async_handler associated with the context. If no user defined async_handler is associated with the context, then an implementation-defined default async_handler is called to handle any errors, as described in [Section 4.13.1.2].

The above wording suggests that only errors produced by commands associated with this event are waiting on are passed to the async handler, however, the use of the phrase "any context that the event was waiting on executions from" suggests that the intention was to pass errors produced by commands of this event and any dependent events.

Summary

In this patch, I have modified the wording of the event::wait_and_throw() member functions to clarify that they will throw asynchronous errors for this event and all dependent events to their respective async handlers.

While changing this I have also modified the wording around the waiting behavior for both the event::wait and event::wait_and_throw member functions to match that which is used in the ISO C++ specification; to state that the function blocks until all commands have been completed. This wording could be extended to also include a synchronization clause, however, since these functions are still using the original format, I've just kept this to the effects clause for now.

Finally, I have modified the wording of the static event::wait() and event::wait_and_throw() functions to be based on the definitions in member functions to avoid duplication of wording.

Target

This patch is targeting SYCL 2020 as a clarification.

[TApplencourt]

Look good to me!

I don't especially like the addition of dependent events for the definition of wait and wait and throw. It seems "obvious/implied " to me, so I don't see why we put it here. But if it's what ISO C++ is doing then it's a good argument to do the same :)

[keryell]

I would prefer the wording wait_or_throw(). :-)

[tomdeakin]

Waiting for changes according to comment #520 (comment)

[etomzak]

What surprises me is that event::wait_and_throw() exists at all. An event is a synchronization point. It makes sense to talk about a queue or a context owning some asynchronous exceptions; I don't think it makes sense to talk about an event owning asynchronous exceptions. IMO event::wait_and_throw() awkwardly mixes two different abstractions.

Options 1, 2, and 3 all look problematic to me, because they assume that async exceptions can be traced on the level of granularity of individual commands. One of the consequences of an asynchronous API (which, in some cases, is also allowed to reorder commands) is that that level of tracing is not possible. This doesn't have to be a bad thing. I'd argue that the minimum sensible level of granularity of async exception tracing is the queue.

So I'll suggest two more possible solutions:

4. Deprecate event::wait_and_throw()
5. Make event::wait_and_throw() equivalent to queue::wait_and_throw() for all queue(s) that the event is associated with

[AerialMantis]

What surprises me is that event::wait_and_throw() exists at all. An event is a synchronization point. It makes sense to talk about a queue or a context owning some asynchronous exceptions; I don't think it makes sense to talk about an event owning asynchronous exceptions. IMO event::wait_and_throw() awkwardly mixes two different abstractions.

Options 1, 2, and 3 all look problematic to me, because they assume that async exceptions can be traced on the level of granularity of individual commands. One of the consequences of an asynchronous API (which, in some cases, is also allowed to reorder commands) is that that level of tracing is not possible. This doesn't have to be a bad thing. I'd argue that the minimum sensible level of granularity of async exception tracing is the queue.

So I'll suggest two more possible solutions:

4. Deprecate event::wait_and_throw()
5. Make event::wait_and_throw() equivalent to queue::wait_and_throw() for all queue(s) that the event is associated with

@etomzak I have responded to this comment in reply to the discussion above.

[AerialMantis]

Look good to me!

I don't especially like the addition of dependent events for the definition of wait and wait and throw. It seems "obvious/implied " to me, so I don't see why we put it here. But if it's what ISO C++ is doing then it's a good argument to do the same :)

Thanks, I agree, this was the original intent so it was implied, but it was raised by users that this was ambiguous so we thought we should clarify the wording to make sure it's clear.

[tomdeakin]

Still waiting on @AerialMantis to update.

[AerialMantis]

I have updated this PR to reflect the clarifications we discussed (3df6485).

[gmlueck]

The new wording looks good to me, but there is an error with the cross reference which causes CI to fail.

[gmlueck]

Summarizing a long WG discussion:

• This PR should loosen the language to say that other async errors could also be reported from wait_and_throw, even if those errors are not directly related to the event or queue being waited upon.
• Otherwise, an implementation must track which command is associated with each async error.
• In the future, we need a longer discussion on async error handling. We do not have good agreement on what the current APIs mean, or what the use case is.
• Adding loose language now that allows an implementation to report extra errors during wait_and_throw does not limit us later. A future spec release can tighten the langue and limit which errors can be returned. This will not be an API break.

[AerialMantis]

I've updated the wording to address the feedback from the recent WG discussion, event::wait_and_throw now allows an implementation to also throw async errors from queues/contexts not used to enqueue the commands the events is waiting on.