Improve description of information descriptors

[Pennycook]

Previously, the description of information descriptors was spread out across multiple sections and appendices. This commit introduces a new section and a glossary entry to more clearly explain the concept of an information descriptor prior to detailing the behavior of each SYCL class.

Closes #410. Addresses feedback originally raised by @nmnobre in #407.

---

I deliberately added this to the end of the "Common interface" section so that it wouldn't affect renumbering, should we choose to cherry-pick it to SYCL 2020.

[TApplencourt]

Always nice to tiding up!

Previously, the description of information descriptors was spread out across multiple sections and appendices.

This is a stupid remark, but should we remove the other text that has now been consolidated in the new section?

[Pennycook]

This is a stupid remark, but should we remove the other text that has now been consolidated in the new section?

I did consider this, but I'd rather leave this to a later PR. I wasn't really sure how far to go with it. I like the way that @gmlueck reformatted the description of the various aspects, and I think it probably makes sense to do something similar with information descriptors. But removing the information descriptor appendix and cleaning up those tables seemed like a huge change.

[TApplencourt]

Work for me!
We are Lanister and will pay our technical debt! (Man, this is an old reference...)

[TApplencourt]

I think the other part of the spec indent has 2 space, but that ok. Need to make a PR to format all the [source] with clang format, so will be fixed then.

[nmnobre]

Thanks a lot for doing this @Pennycook.
I think it's a very good readability improvement. :)

[gmlueck]

I'm not wild about this change for a few reasons:

• The new section "Information queries" is not really normative. Instead, it describes a general pattern that some of the other classes follow. However, those other classes have a normative description of their information descriptors. Therefore, the new section seems redundant to me.

• The new section "Information queries" has a list of classes that follow this pattern. We will need to remember to update this list if we ever add information descriptors to other classes.

• I especially don't like the new references to "Appendix A" because my goal is to remove that appendix.

I agree with @nmnobre that the old format we had in the SYCL specification made the structure of information descriptors non-obvious, just as he says in #407. However, I think the new format fixes this because it shows the exact definition of each information descriptor. We use this format for most of the classes that use information descriptors (context, device, platform, queue). This new format did not exist when #407 was filed.

The only classes with information descriptors that don't follow the new format are event and kernel. I think that the right fix for #407 should be to update these sections to the new format, and then we can remove appendix A.

[nmnobre]

However, I think the new format fixes this because it shows the exact definition of each information descriptor. [...]
The only classes with information descriptors that don't follow the new format are event and kernel. I think that the right fix for #407 should be to update these sections to the new format, and then we can remove appendix A.

Ah! This is awesome, I wasn't aware of this reformat! For clarity, you're talking about #567, #587, #601, #606 (and their cherry-picks to sycl-2020), correct? I agree the new format is sufficient and, since it's already in sycl-2020, am I correct in assuming the content of this PR will never be published before or without that new format? If that's the case, it might be redundant, yeah. And I now understand, so is #729?

The only caveat is if we don't change event and kernel before the new publishing date (would that be rev. 10?) in which case it'd probably make sense to keep both changes - not sure...?

[Pennycook]

• The new section "Information queries" is not really normative. Instead, it describes a general pattern that some of the other classes follow. However, those other classes have a normative description of their information descriptors. Therefore, the new section seems redundant to me.
• The new section "Information queries" has a list of classes that follow this pattern. We will need to remember to update this list if we ever add information descriptors to other classes.

I was following the precedent of set by Section 4.5.2, Section 4.5.3 and Section 4.5.4, all of which describe a general pattern that other classes follow and provide a list of classes. Are you suggesting that we get rid of these sections too?

I think there is value in providing a high-level overview of why commonality may exist in an interface like this. I agree with you that showing the actual interface and listing the classes is probably unnecessary, but I'd like us to be consistent.

To clarify: I'm not opposed to taking the interface and class list out of this PR, leaving only the introduction to the concept of an information descriptor and the glossary entry. But if we do that, I think it would make sense to define all of the operators available in SYCL runtime classes explicitly, instead of just saying something like "This class has common reference semantics."

• I especially don't like the new references to "Appendix A" because my goal is to remove that appendix.

I agree we should remove the appendix. I can remove the references to the appendix, to make that easier.

The only classes with information descriptors that don't follow the new format are event and kernel. I think that the right fix for #407 should be to update these sections to the new format, and then we can remove appendix A.

I agree we should do this.

[gmlueck]

For clarity, you're talking about #567, #587, #601, #606 (and their cherry-picks to sycl-2020), correct?

Yes.

am I correct in assuming the content of this PR will never be published before or without that new format?

Correct.

And I now understand, so is #729?

We can keep #729 if you want. The changes it makes to Appendix A are valid. I hope to remove Appendix A at some point, but it might not happen for rev 10.

The only caveat is if we don't change event and kernel before the new publishing date (would that be rev. 10?) in which case it'd probably make sense to keep both changes - not sure...?

I don't mind keeping #729. It only changes Appendix A, and it's easy to remove all of Appendix A later. I'm not wild about the changes here in #725 because it adds more stuff to the rest of the spec that we'd need to remove later.

I was following the precedent of set by Section 4.5.2, Section 4.5.3 and Section 4.5.4, all of which describe a general pattern that other classes follow and provide a list of classes. Are you suggesting that we get rid of these sections too?

I think those sections are different. They do more than just explain a code pattern. They are the normative description of the "common reference semantics" member functions, etc. For example, the queue class doesn't even list these member functions in its synopsis. It just contains a comment "common interface members", which I guess is supposed to mean that this class defines the member functions specified in the common reference semantics section.

I'm not sure what the right thing to do here is yet. I think it might be more clear to explicitly list all the common reference semantics member functions, and then have a description that says something like:

See section XX for a description of these functions.

where section XX defines the common reference semantics.

I don't think we need to do this for get_info. It's easy to describe what the function does along with all the other member functions. I don't think it would make the spec any clearer to have a common section describing get_info. In fact, I think it makes the spec less clear because we want the Constraints, etc. to be tailored for each class.

To clarify: I'm not opposed to taking the interface and class list out of this PR, leaving only the introduction to the concept of an information descriptor and the glossary entry.

If you remove the interface and the class list, what would remain in the section "Information queries"? I'd like to avoid listing all the classes that define get_info for the reason I mention above. It seems like this leaves very little in the section.

[Pennycook]

I'm not sure what the right thing to do here is yet. I think it might be more clear to explicitly list all the common reference semantics member functions, and then have a description that says something like:

See section XX for a description of these functions.

where section XX defines the common reference semantics.

I agree that this would be clearer. The first time I read the SYCL specification, I thought the comment that says "Common member functions" was a header for the functions beneath it, rather than a substitute for some function definitions.

I don't think we need to do this for get_info. It's easy to describe what the function does along with all the other member functions. I don't think it would make the spec any clearer to have a common section describing get_info. In fact, I think it makes the spec less clear because we want the Constraints, etc. to be tailored for each class.

If you remove the interface and the class list, what would remain in the section "Information queries"? I'd like to avoid listing all the classes that define get_info for the reason I mention above. It seems like this leaves very little in the section.

I've pushed a change in c31cfca to show what this could look like.

As you say, it does end up being quite a small section. But I still think there's some value in highlighting somewhere that there's a single get_info() function design that is shared by a bunch of classes. If a first time reader starts with the early sections before diving into the class definitions, it will teach them to expect that several classes expose this function. Without a section like this, a reader won't realize get_info() is a common interface until they've read at least two class definitions.

[gmlueck]

A couple comments on this one. Feel free to merge this when you think it's ready.

This is probably my last review for a while as I have finally finished my travels and have arrived in New Zealand!

[gmlueck]

This isn't entirely true because there is also get_backend_info, which also takes an information descriptor.

[Pennycook]

Good catch! I always forget about this backend-specific stuff. I've added references to backends in 2aa55a0.

[gmlueck]

Why is the return type std::string? Just because a "name" would normally be a string?

[Pennycook]

Right, exactly.

I've tried to reword this glossary entry in b70832f to avoid this issue: instead of saying that it "would" return a std::string just because it's called "name", I've flipped it to say that if a descriptor was called "name" and had a return type defined to be std::string, the information query should be expected to return the entity's name as a string.