Clarify sections previously using "synchronize" #484
Conversation
ISO C++ defines a "synchronizes-with" relationship between library calls, and defines functions in terms of their "synchronization" effects. Additionally, it refers to the new std::barrier and std::latch types as "coordination mechanisms" rather than "synchronization mechanisms". Before this PR, several usages of "synchronize" (or "synchronization") were incompatible with these definitions, or at least potentially ambiguous. For example, several sections talked about "synchronizing" host and device data, but really meant that data would be copied from one place to another. Other sections talked about work-items or different devices "synchronizing with" one another, but without reference to specific library calls. This PR rewords several sections, as follows: - Where "synchronize" was used to mean "wait", the specification now says that the caller "blocks" until a certain condition holds. - Where "synchronize" was used to mean "two copies of data are made consistent", the specification now says that data is "copied". - Where "synchronize" was used to mean "call a barrier", the specification now talks about "coordination" of work-items and/or devices. Any remaining occurrences of "synchronize" are related to mutexes (which ISO C++ describes as a "synchronization primitive") or to atomics and fences (which ISO C++ describes as "synchronizing with" other atomics and fences). Closes internal issue 629.
|
A quick note to reviewers: there were also a few places where "synchronize" was used in the glossary. I was surprised that there were such lengthy descriptions for these terms in the glossary, as they could very easily get out of sync with the main text. Rather than rewrite these glossary descriptions to match the updated text, I simplified them and pointed back to where the relevant behaviors are specified. |
|
For Or maybe something like: |
I don't think we should redefine coordination to mean synchronization. I'm worried that will confuse people more, because they're not exactly the same. I think this specific usage of coordination really follows the English meaning, and just means something like "getting the host and device to work together effectively". I'm not opposed to swapping it out for a different word. The only usage of "coordinate" that I feel strongly about keeping is the description of barriers as "a coordination mechanism". |
|
Oh sorry, I was not clear. I'm highly supportive of following the C++ terminology! It's maybe just me, but reading the new My suggestion was just to add a sentence to try to "ease" the reading to non-CS / C++ experts. But maybe it's not the goal of the specification to do so. I will let other people chime in. Regardless of this, It's a nice change and improves the spec clarity which is always good! |
No need to apologize -- you were perfectly clear! I'm not opposed to explaining what things mean, I was just trying to point out that 'coordination ("synchronization")' might leave a reader with the impression that the terms were equivalent.
You're not alone. I hadn't seen "coordination" used to describe barriers until I saw it in C++20. My understanding of how things are defined in C++20 is that there's a sort of hierarchy of concepts. A barrier provides a way to manage/organize/coordinate a set of threads. The implementation of the barrier guarantees that all calls to a function like I deliberately went through and tried to change every usage of "synchronization" where I wasn't 100% sure it was matching the ISO C++ definition. If we can convince ourselves that I've been too heavy-handed here, and that some functions in SYCL do provide synchronization, then I'm more than happy to revisit the text. I expected we'd have to discuss this PR for quite some time, so please keep the questions coming!
Where I'd like us to end up eventually is even closer to ISO C++, so each function has its own Synchronization: section that says what synchronization operations (if any) are involved. So, the description of Would that help? I meant to write something about this in the PR description, but forgot. |
Yes! Then, perfect, we can merge this as-is and eagerly wait for the next PR :). I guess my comment boiled down to more of a philosophical point. I would like to keep the spec readable for mere mortals. Of course, clarify / precision / unambiguous terms are the critical things in a spec! No debate about it. But if we can avoid becoming like some other specs that are totally unreadable by non-implementers, I will appreciate it. I guess I want a "SYCL Spec for Dummies" sentences from time to time :) |
Consistent with the rest of the sentence. Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
Descriptions of accessors elsewhere in the specification say that functions block "until data is available", as not all implementations of accessors are guaranteed or required to copy data.
| @@ -5961,7 +5962,7 @@ property::image::use_mutex | |||
| application via a [code]#std::mutex# provided to the | |||
| property. The [code]#std::mutex# is locked by | |||
| the runtime whenever the data is in use and unlocked | |||
| otherwise. Data is synchronized with [code]#hostData#, when | |||
| otherwise. Data is copied back to [code]#hostData# when | |||
There was a problem hiding this comment.
The way this is worded suggests that data will always be copied back once the mutex is unlocked, but if the buffer would ordinarily wait on commands accessing that data on destruction it will also coordinate, and if the buffer would ordinarily not wait, then there will be no copy back.
There was a problem hiding this comment.
I don't think I understand how to fix this, primarily because I still don't think I understand how all the buffer destruction rules work. Could you suggest alternative wording to fix this?
There was a problem hiding this comment.
I think we could rewrite that last sentence to read:
The contents of
hostDataare guaranteed to reflect contents of the image when thestd::mutexis unlocked by the runtime.
Note that this paragraph describes the use_mutex property for images. I think there is a similar paragraph earlier in the spec that describes the use_mutex property for buffers. That should also be changed.
There was a problem hiding this comment.
I've made both changes in d341f85.
There was a problem hiding this comment.
I think this wording makes sense, though I think we should also clarify that this only occurs if there is a host pointer to copy to.
There was a problem hiding this comment.
Is there any reason to use use_mutex if there isn't a host pointer? I've not used it, but my understanding from reading the spec was that it's only useful to synchronize access to hostData.
There was a problem hiding this comment.
Not really, though technically it is possible to create a buffer with use_mutex and without a host pointer and mutex would be on any host allocation for the memory, you could also use set_final_data to set a host pointer.
Saying that I think it's fair to assume that it's implicit that the contents of hostData is only reflected if a host pointer is provided, so I think the wording is fine as it is.
Also add a brief paragraph to explain the connection between coordination and synchronization.
After thinking about this some more, I've renamed the section to "Coordination and Synchronization", and added a paragraph that attempts to connect the two. See 76978a5. |
Co-authored-by: Gordon Brown <gordon@codeplay.com>
Co-authored-by: Gordon Brown <gordon@codeplay.com>
|
@Pennycook: we decided in the WG that we can merge this PR once @AerialMantis approves it (indicating he is happy with your resolution of his concerns). I will hold off merging #481 until this happens. It looks like there are merged conflicts that need to be resolved, so please do that also @Pennycook. |
Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
Clarify sections previously using "synchronize"
Clarify sections previously using "synchronize" (cherry picked from commit 463a26c)
Clarify sections previously using "synchronize" (cherry picked from commit 463a26c)
ISO C++ defines a "synchronizes-with" relationship between library calls, and defines functions in terms of their "synchronization" effects. Additionally, it refers to the new std::barrier and std::latch types as "coordination mechanisms" rather than "synchronization mechanisms".
Before this PR, several usages of "synchronize" (or "synchronization") were incompatible with these definitions, or at least potentially ambiguous. For example, several sections talked about "synchronizing" host and device data, but really meant that data would be copied from one place to another. Other sections talked about work-items or different devices "synchronizing with" one another, but without reference to specific library calls.
This PR rewords several sections, as follows:
Where "synchronize" was used to mean "wait", the specification now says that the caller "blocks" until a certain condition holds.
Where "synchronize" was used to mean "two copies of data are made consistent", the specification now says that data is "copied".
Where "synchronize" was used to mean "call a barrier", the specification now talks about "coordination" of work-items and/or devices.
Any remaining occurrences of "synchronize" are related to mutexes (which ISO C++ describes as a "synchronization primitive") or to atomics and fences (which ISO C++ describes as "synchronizing with" other atomics and fences).
Closes internal issue 629.