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.

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

Contradictions with context constructors #470

Open
gmlueck opened this issue Oct 9, 2023 · 2 comments · May be fixed by #628
Open

Contradictions with context constructors #470

gmlueck opened this issue Oct 9, 2023 · 2 comments · May be fixed by #628

Comments

@gmlueck
Copy link
Contributor

gmlueck commented Oct 9, 2023

I discovered this problem when reviewing work on the SYCL Reference guide (KhronosGroup/SYCL_Reference#73). I'd like to get a quick response from the WG on this, so that the reference guide can document the correct information.

The SYCL 2020 specification of the context constructors is contradictory. The synopsis lists these constructors:

explicit context(const property_list& propList = {});

explicit context(async_handler asyncHandler,
                 const property_list& propList = {});

explicit context(const device& dev, const property_list& propList = {});

explicit context(const device& dev, async_handler asyncHandler,
                 const property_list& propList = {});

explicit context(const std::vector<device>& deviceList,
                 const property_list& propList = {});

explicit context(const std::vector<device>& deviceList,
                 async_handler asyncHandler,
                 const property_list& propList = {});

While Table 19 lists these constructors:

explicit context(async_handler asyncHandler = {});

explicit context(const device& dev, async_handler asyncHandler = {});

explicit context(const std::vector<device>& deviceList,
                 async_handler asyncHandler = {});

Looking at the history of the specification, I see this same contradiction in SYCL 1.2.1.

I also noticed that we mysteriously removed two constructors that used to exist in SYCL 1.2.1:

context(const platform &plt, const property_list &propList = {});

context(const platform &plt, async_handler asyncHandler, const property_list &propList = {});

I can't think of any reason why we would want to remove these constructors, and there is no mention about removing them in the What's changed chapter. Therefore, I think this was probably just a mistake on our part. Looking at the history, I see that these constructors were also removed (for no apparent reason) in the SYCL 2020 provisional specification.

I'd like to fix the SYCL 2020 specification to fix the contradiction and restore the two constructors that were removed from SYCL 1.2.1. I believe the constructors listed in the synopsis are the correct ones, and the ones listed in Table 19 are wrong. Therefore, I think this should be the complete list of context constructors in SYCL 2020:

explicit context(const property_list& propList = {});

explicit context(async_handler asyncHandler,
                 const property_list& propList = {});

explicit context(const device& dev, const property_list& propList = {});

explicit context(const device& dev, async_handler asyncHandler,
                 const property_list& propList = {});

explicit context(const platform &plt, const property_list &propList = {});

explicit context(const platform &plt, async_handler asyncHandler, const property_list &propList = {});

explicit context(const std::vector<device>& deviceList,
                 const property_list& propList = {});

explicit context(const std::vector<device>& deviceList,
                 async_handler asyncHandler,
                 const property_list& propList = {});

If there are no objections, I volunteer to fix the spec.

@TApplencourt
Copy link
Contributor

TApplencourt commented Oct 9, 2023

Agree about fixing the inconsistency between the table and synopsis (ofc!)

Concerning the platform,

context(const platform &plt, const property_list &propList = {});
context(const platform &plt, async_handler asyncHandler, const property_list &propList = {});

Seems to be just syntactic sugar over

context( plt.get_devices(), const property_list &propList = {});
context( plt.get_devices(), async_handler asyncHandler, const property_list &propList = {});

Maybe the goal was to trim down the API surface? Putting them back adds to the complexity of the empty platform proposal. Regardless, I have no strong opinion. Maybe A little preference of putting them back, just for backward compatibility

@gmlueck
Copy link
Contributor Author

gmlueck commented Oct 9, 2023

Yes, I also think we should restore those two constructors for backwards compatibility. I think our intention with SYCL 2020 was to retain backward compatibility with SYCL 1.2.1 unless there was some overriding reason that required breaking compatibility. I don't see any overriding reason in this case.

gmlueck added a commit to gmlueck/SYCL-Docs that referenced this issue Sep 20, 2024
The context constructor overloads in the `context` synopsis did not
match the description in the text of the specification.  We decided that
the overloads in the synopsis were what we intended, so change the text
to match.

We also discovered that two constructor overloads taking a `platform`
argument were present in SYCL 1.2.1 and were inadvertently dropped from
the SYCL 2020 specification.  Add them back.

Clarify the behavior of the constructors when the set of devices is
empty.  We decided that this should throw an exception, with is
consistent with OpenCL's `clCreateContext`.

Closes KhronosGroup#470
Closes KhronosGroup#474
gmlueck added a commit to gmlueck/SYCL-Docs that referenced this issue Sep 20, 2024
The context constructor overloads in the `context` synopsis did not
match the description in the text of the specification.  We decided that
the overloads in the synopsis were what we intended, so change the text
to match.

We also discovered that two constructor overloads taking a `platform`
argument were present in SYCL 1.2.1 and were inadvertently dropped from
the SYCL 2020 specification.  Add them back.

Clarify the behavior of the constructors when the set of devices is
empty.  We decided that this should throw an exception, which is
consistent with OpenCL's `clCreateContext`.

Closes KhronosGroup#470
Closes KhronosGroup#474
@gmlueck gmlueck linked a pull request Sep 20, 2024 that will close this issue
gmlueck added a commit to gmlueck/SYCL-Docs that referenced this issue Dec 5, 2024
The context constructor overloads in the `context` synopsis did not
match the description in the text of the specification.  We decided that
the overloads in the synopsis were what we intended, so change the text
to match.

We also discovered that two constructor overloads taking a `platform`
argument were present in SYCL 1.2.1 and were inadvertently dropped from
the SYCL 2020 specification.  Add them back.

Clarify the behavior of the constructors when the set of devices is
empty.  We decided that this should throw an exception, which is
consistent with OpenCL's `clCreateContext`.

Closes KhronosGroup#470
Closes KhronosGroup#474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants