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

Accessor aliases #100

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ProGTX
Copy link
Contributor

@ProGTX ProGTX commented Sep 22, 2019

In order to reduce the verbosity of programming with SYCL accessors,
this proposal aims to reduce the number of template parameters
to just 2 from the current 5.
It achieves this by slightly revising
how to treat read-only data
and adding alias templates
based on the access target.

  1. Main changes
    • Default accessor template parameters
    • Simplify access modes
    • Alias templates based on access target
    • Extend the handler class
  2. Accessor alias templates
    • `constant_buffer_accessor
    • host_accessor
  3. Treat const T the same as access::mode::read
    • Simplifies a lot of code
  4. Define implicit conversions for equivalent types
  5. Implicit conversions that add const
  6. Overload for handler::require that also takes an access mode
  • To weaken the access mode
  1. Default all accessor template parameters
  • Expect the data type
  • Default to an accessor to global buffer with read-write access
  • Assumes accessors can be a placeholder without template parameter
  1. Discussed some considerations and alternatives
  2. Examples of reduced verbosity

Copy link
Contributor

@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.

That looks like a good simplification.
You could also add some CTAD if compiled with C++17 or higher.

@ProGTX
Copy link
Contributor Author

ProGTX commented Mar 9, 2020

That looks like a good simplification.
You could also add some CTAD if compiled with C++17 or higher.

@keryell I've greatly simplified the proposal, and I've added a section on CTAD. I think I would need to add deduction guides to the accessor class, I might still add that. There's also the problem that CTAD doesn't work on alias templates in C++17, so maybe I should add something for C++20 as well?

@keryell
Copy link
Contributor

keryell commented Mar 10, 2020

That looks good. Probably you need to coordinate with Intel to unify the simplifications they are pushing for too.

Copy link
Contributor

@AerialMantis AerialMantis left a comment

Choose a reason for hiding this comment

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

Looks good thanks! Just a couple of comments.

accessor-alias/sycl-2.2/index.md Show resolved Hide resolved

## Defaulting template parameters

We propose defaulting all accessor template parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

This could conflict with the changes proposed in KhronosGroup/SYCL-Docs#73 to introduce an optional property_list to thee accessor constructor. Though perhaps some of the information that we currently pass such as the access::mode information that is not needed for the type-system could be passed as a property?

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 quite see how that conflicts, this proposal is about defaulting template parameters, not constructor arguments. Though it's possible some extra deduction guides might be needed because of those changes.

});
```

We also propose the following rules for simplifying the access mode:
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be worth making the discard part of the access::mode a separate runtime parameter or property?

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 was considering that, but it seems like the existing access modes work reasonably well with this proposal, so a new tag might actually be adding verbosity.

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've added property::discard and property::discard_v

In order to reduce the verbosity of programming with SYCL accessors,
this proposal aims to reduce the number of template parameters
to just 2 from the current 4 (placeholders accessors even have 5).
It achieves this by adding aliases to accessors
based on the access target
and slightly revised rules for access modes.

1. Added aliases to accessors
  * `buffer_accessor`,
  * `constant_buffer_accessor
  * `host_accessor`
2. Deduce access mode based on constness of data type
  * `access::mode::read` for `const dataT`
  * `access::mode::read_write` for `dataT`
3. Allow conversions to new aliases
4. Return aliases from buffer access
  * `get_device_access`
  * `get_device_constant_access`
  * `get_host_access`
5. Allow any accessor to be registered with `handler::require`
6. Overload for `handler::require` that also takes an access mode
  * Hint to the scheduler
7. Type traits to help with deducing access modes
  * `access_mode_from_type`
  * `type_from_access_mode`
8. Default all accessor template parameters
  * Expect the data type
  * Default to an accessor to global buffer with read-write access
  * Assumes any accessor can be a placeholder
9. Discussed some considerations and alternatives
10. Examples of reduced verbosity
In order to reduce the verbosity of programming with SYCL accessors,
this proposal aims to reduce the number of template parameters
to just 2 from the current 5.
It achieves this by slightly revising
how to treat read-only data
and adding alias templates
based on the access target.

1. Main changes
    * Default accessor template parameters
    * Simplify access modes
    * Alias templates based on access target
    * Extend the handler class
2. Accessor alias templates
    * `constant_buffer_accessor
    * `host_accessor`
3. Treat `const T` the same as `access::mode::read`
    * Simplifies a lot of code
4. Define implicit conversions for equivalent types
5. Implicit conversions that add `const`
6. Overload for `handler::require` that also takes an access mode
  * To weaken the access mode
8. Default all accessor template parameters
  * Expect the data type
  * Default to an accessor to global buffer with read-write access
  * Assumes accessors can be a placeholder without template parameter
9. Discussed some considerations and alternatives
10. Examples of reduced verbosity
In order to reduce the verbosity of programming with SYCL accessors,
this proposal aims to reduce the number of template parameters
to just 2 from the current 5.
It achieves this by slightly revising how to treat read-only data
and adding alias templates based on the access target.

1. Main changes
    * Default accessor template parameters
    * Simplify access modes
    * Alias templates based on access target
    * Extend the handler class
2. Accessor alias templates
    * `constant_buffer_accessor
    * `host_accessor` as a new type
3. Treat `const T` the same as `access::mode::read`
    * Simplifies a lot of code
4. Define implicit conversions for equivalent types
5. Implicit conversions that add `const`
6. Added `property::discard`
7. Overload for `handler::require` that also takes `property::discard`
    * To ignore previous data
8. Default all accessor template parameters
    * Expect the data type
    * Default to an accessor to global buffer with read-write access
    * Assumes accessors can be a placeholder without template parameter
9. Good support for CTAD
    * Deduction tags as compile-time properties
    * Made all accessor constructors variadic templates
10. Discussed some considerations and alternatives
11. Examples of reduced verbosity
@ProGTX
Copy link
Contributor Author

ProGTX commented Apr 6, 2020

I've pushed revision 0.3, which focuses a lot more on CTAD and makes host_accessor a separate type

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 this pull request may close these issues.

3 participants