Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify default context behavior #623

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Clarify default context behavior #623

wants to merge 5 commits into from

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Sep 17, 2024

Clarify that the queue constructors that do not take a context parameter use the platform's default context.

The WG decided to make this as a clarification to the SYCL 2020 specification because:

  • DPC++ has always behaved this way, and AdaptiveCpp mostly behaves this way now.

  • It would be difficult to enable this as an extension because it doesn't add any new API. Therefore, an application could end up relying on the "default context" behavior without the author of the application even being aware that the application is using extended behavior.

@sycl-issue-bot sycl-issue-bot bot mentioned this pull request Sep 17, 2024
Open
@gmlueck
Copy link
Contributor Author

gmlueck commented Sep 17, 2024 •
edited
Loading

NOTE This PR is "stacked" on top of #606. Once #606 is merged, I'll retarget this PR to the "main" branch.

EDIT #606 has been merged now, so I retargeted this PR to the "main" branch.

@TApplencourt
Copy link
Contributor

Potential rephrasing (which was a little simpler to read for me)

All [code]#queue# objects have an associated device, platform, and context which
are determined by the constructor.

The description of each constructor below tells how the associated device is chosen.
The associated platform is always the platform that contains this device.
When the queue constructor does not take a context object as a parameter, the queue is associated to the <<platform's default context>>.

And move <<platform's default context>>. to the platform section?

## Platform's default context

Each platform has a single context object which is used as its default context.
That context object contains all of the devices in the platform.

We repeat the The queue has the default context of the platform that contains this device. all the time.

So IMO we can remove the When the queue constructor does not take a context object as a parameter, the queue is associated to the <<platform's default context>>. then.

Or is this a tradeoff between duplicating the information and convenience/clarity for reader of the spec?

@gmlueck
Copy link
Contributor Author

gmlueck commented Sep 19, 2024

OK. How about this:

  • Remove the big paragraph entirely (the one starting with "All queue objects have an associated device..."

  • Add a short paragraph to the "Platform class" section defining the default context as you suggest.

  • Change the descriptions of the constructors like:

    Effects: Constructs a queue object using the device selected by default_selector_v. The queue's platform is
    the platform that contains this device. The queue's context is this platform's default context as described in section X.Y.

@TApplencourt
Copy link
Contributor

I like it!

@gmlueck gmlueck mentioned this pull request Sep 20, 2024
Open
keryell
keryell approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@gmlueck
Clarify default context behavior
5f812f7 
Clarify that the `queue` constructors that do not take a `context`
parameter use the platform's default context.

The WG decided to make this as a clarification to the SYCL 2020
specification because:

* DPC++ has always behaved this way, and AdaptiveCpp mostly behaves this
  way now.

* It would be difficult to enable this as an extension because it
  doesn't add any new API.  Therefore, an application could end up
  relying on the "default context" behavior without the author of the
  application even being aware that the application is using extended
  behavior.
@gmlueck
Reword as suggested by Thomas
9075baa
tomdeakin
tomdeakin reviewed Oct 3, 2024
View reviewed changes
adoc/chapters/programming_interface.adoc Outdated
@@ -969,6 +969,9 @@ The [code]#platform# class also provides constructors, but constructing a new
[code]#platform# instance merely creates a new object that is a copy of one of
the objects returned by [api]#platform::get_platforms#.

Each platform has a single context object which is used as its default context.
Copy link
Contributor

@tomdeakin tomdeakin Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@illuhad Can you confirm if a single object is OK? Intent should be compare equal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmlueck to update!

Copy link
Contributor Author

@gmlueck gmlueck Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the wording in 2d63525.

Note that I also changed the wording to say that the default context has no async error handler. I presume that is our intent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the Each platform has a single context . A platform has no context; a context has a platform, not the other way around.

I would prefer the term "is associated to".

Applications can retrieve a copy of this default context object, for example,
by constructing a queue.

Maybe something like "A copy of 'default context' will be used when a context is implicitly created (for example by a the queue constructor (1))." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the Each platform has a single context . A platform has no context; a context has a platform, not the other way around.

The text you quote was removed in 2d63525. The new text is:

Each platform has a default context which contains all of the devices in the platform and has no associated asynchronous error handler.

Do you object to that text?

Maybe something like "A copy of 'default context' will be used when a context is implicitly created (for example by a the queue constructor (1))." ?

How about this:

Certain operations require the platform's default context to be created. For example, this happens for some of the queue constructors. When a platform's default context is created, it behaves as though it is a copy of a single internal context object that is associated with each platform.

Copy link
Contributor

@TApplencourt TApplencourt Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my bad.

Do you object to that text?

Yes, I don't like the has. It sounds like an implementation leak to my ear. For me as == member variable

It's not like each platform has a default context.

It's more that when a queue is created, we will deduce a platform (following the rule in the table). Then, from this platform, we instantiate a context. This context is (copy?) an the instance of a singleton who have all the device of the platform.

So I will prefer is associated with.

When a platform's default context is created, it behaves as though it is a copy of a single internal context object that is associated with each platform.

What about:

Certain operations require a context to the implicitly created. For example, this happens for some of the queue constructors. The context associated with this queue is a copy of the default context associated with the platform of this queue.

Sorry for so much nitpicking... If I'm still not clear,I can accept your change, no problem. It's already better than before!

@gmlueck gmlueck changed the base branch from gmlueck/reformat-queue to main October 3, 2024 18:40
@gmlueck gmlueck mentioned this pull request Oct 7, 2024
Closed
@gmlueck
Reword description of default context
2d63525 
Reword to address comments from WG.

I also noticed that we never state anything about the async error
handler of the default context object.  I'm pretty sure our intent
is that the default `context` object has no async error handler, so
I added words to this effect.
@gmlueck
Fix reflow
013b3a8
@gmlueck
Reword description of default context again
da50947
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomdeakin tomdeakin tomdeakin approved these changes

@keryell keryell keryell approved these changes

@TApplencourt TApplencourt TApplencourt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gmlueck @TApplencourt @tomdeakin @keryell