Align discussion of forward progress with C++#300
Align discussion of forward progress with C++#300keryell merged 11 commits intoKhronosGroup:SYCL-2020/masterfrom
Conversation
The strongest forward progress guarantee in ISO C++ is called "concurrent forward progress". The phrase "execute concurrently" can easily be misinterpreted as "execute with concurrent forward progress guarantees" and so it is safer to avoid it.
Using ISO C++ terminology, it is more precise to say that work-items provide weakly parallel forward progress guarantees.
In ISO C++, the forward progress guarantees provided by a thread of execution are a property of that thread of execution alone. The wording prior to this commit implied that the forward progress guarantees of one work-item needed to be interpreted relative to another work-item.
Earlier edits had introduced some repetition. The fact that work-items are threads of execution with weakly forward progress guarantees is arguably the most important information in this section, so it should come first.
Imports only the terms that are currently used by the SYCL specification, to avoid unintended redefinitions and/or conflicts.
Explains the implications of weakly parallel forward progress guarantees and forward progress guarantee delegation in a more accessible way.
Clarifies what it means for an implementation to "obey the semantics of group barriers".
adoc/chapters/architecture.adoc
Outdated
| * the work-items being synchronized both provide concurrent forward progress | ||
| guarantees. |
There was a problem hiding this comment.
Some interpretations of "synchronize" probably don't require concurrent forward progress guarantees. Should this be strengthened to "synchronizes with", and the term imported from C++ above in L1101-1105 ?
There was a problem hiding this comment.
You're right, this section is unclear. Reading this again, I think we might even be conflating two concepts.
The first bullet point definitely means "synchronizes with". A device needs to support acquire-release ordering or sequential consistency in order to have atomics from different work-items synchronize with one another.
The second bullet point is a more colloquial usage of "synchronized", and I think it's actually talking about the situation where one work-item attempts to synchronize with another using an atomic operation that is not "lock-free", or which otherwise blocks. It's the blocking nature of the atomic operation that requires strong forward progress guarantees.
I'm not sure what to do here. If we import "synchronizes with" and "lock-free" we potentially have to rewrite large parts of the specification (e.g. the section currently titled "Synchronization"!). We might want to do that eventually, but it seems like something we could tackle separately.
Another option would be to drop these bullet points from the specification temporarily and reintroduce them later. Since they're only offering guidance to developers, it may make more sense to have them as non-normative notes in a different section.
There was a problem hiding this comment.
I've drafted a change in 3a5e790 to show what the last option would look like. I think this fixes the immediate issue of the paragraph you pointed to being confusing.
I'll open an internal issue to discuss "synchronizes-with" further.
The note in the forward progress section conflated work-items "synchronizing with" one another (via release-acquire) with work-items calling blocking atomic operations. This commit rewrites the note for clarity, moves it to the section describing atomic references, and makes it non-normative.
Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
tomdeakin
left a comment
There was a problem hiding this comment.
Minor point, but otherwise it think this is a good clarification.
adoc/chapters/architecture.adoc
Outdated
| All implementations must additionally ensure that the work-items in a group | ||
| obey the semantics of <<group-barrier,group barriers>>: when a work-item | ||
| arrives at a group barrier acting on group _G_, implementations must eventually | ||
| select and potentially strengthen another work-item in group _G_ that has not |
There was a problem hiding this comment.
| select and potentially strengthen another work-item in group _G_ that has not | |
| select and potentially strengthen the progress guarantees of another work-item in group _G_ that has not |
There was a problem hiding this comment.
My motivation for this comment is SYCL programmers might not be experts in the C++ forward progress, but we should try to give as many helpful hints here to make it easy to read the spec, write SYCL, and dive into C++ later if they want.
There was a problem hiding this comment.
I agree with the comment about readability, but I'm less sure about the fix.
I modelled this on the language used by the C++ draft, which always talks about strengthening a thread of execution, rather than strengthening a thread of execution's progress guarantees. I'd like to try and avoid introducing minor differences in wording if we can, just to be safe.
If we rewrote this paragraph to explain the semantics of group barriers in words, would it be clear enough? Something like:
All implementations must additionally ensure that a work-item arriving at a
<<group-barrier, group barrier>> does not prevent other work-items in the same
group from making progress. When a work-item arrives at a group barrier acting
on group G, implementations must eventually select and potentially strengthen
another work-item in group G that has not yet arrived at the barrier.
There was a problem hiding this comment.
Fair enough - better to follow the C++ wording. The new paragraph looks fine to me too.
There was a problem hiding this comment.
Sorry, it's just I've been burnt enough times by unintentionally misusing words like "synchronize" that it's made me paranoid. 😆
I've put the new paragraph in 386749c. Thanks again for pointing out the readability/accessibility issue here.
Align discussion of forward progress with C++
The commits here implement the clarifications and bug fixes that were discussed during the F2F.
A quick summary of the changes below:
Addresses internal issues 311 and 627.