Skip to content

[SYCL][DOC] Add extension for annotated_ptr class and its properties #5755

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
Merged
merged 57 commits into from
Dec 15, 2022
Merged

[SYCL][DOC] Add extension for annotated_ptr class and its properties #5755

merged 57 commits into from
Dec 15, 2022

Conversation

tiwaria1
Copy link
Contributor

@tiwaria1 tiwaria1 commented Mar 8, 2022 •
edited
Loading

Introduce three new extensions:

  1. sycl_ext_oneapi_annotated_ptr
    Introduces a pointer wrapper class that provides a mechanism to attach compile-time constant information to a pointer in a manner that allows the compiler to reliably maintain and analyze the information.

  2. sycl_ext_intel_fpga_kernel_arg_properties
    Defines additional supported properties for annotated_ptr and annotated_arg classes. This commit will remove the extension sycl_ext_intel_fpga_annotated_arg_properties.

  3. sycl_ext_oneapi_kernel_arg_restrict_property
    Defines the restrict property for annotated_ptr and annotated_arg classes in a separate extension rather than including it in sycl_ext_intel_fpga_kernel_arg_properties so that vendors can support the property without having to support FPGA specific properties.

This commit also updates the annotated_arg extension to remove function overloads that are not needed.

@tiwaria1
Copy link
Contributor Author

tiwaria1 commented Mar 8, 2022 •
edited
Loading

Working on:

  • Double check that extension aligns with latest template.
  • Update the properties syntax based on recent changes to the design of compile-time properties.

gmlueck
gmlueck reviewed Mar 8, 2022
View reviewed changes
@gmlueck gmlueck mentioned this pull request Mar 8, 2022
Closed
Pennycook
Pennycook requested changes Mar 9, 2022
View reviewed changes
@sherry-yuan sherry-yuan mentioned this pull request Mar 18, 2022
Open
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
GarveyJoe
GarveyJoe reviewed Apr 6, 2022
View reviewed changes
@tiwaria1
Copy link
Contributor Author

tiwaria1 commented Apr 13, 2022 •
edited
Loading

@gmlueck , @GarveyJoe Please do another round of review. I have addressed your comments and added FPGA properties as a separate extension.

dm-vodopyanov
dm-vodopyanov reviewed Oct 24, 2022
View reviewed changes
sycl/doc/extensions/proposed/sycl_ext_oneapi_annotated_arg.asciidoc Outdated
@@ -23,7 +23,7 @@
== Notice

[%hardbreaks]
Copyright (c) 2021-2022 Intel Corporation. All rights reserved.
Copyright (c) 2022-2022 Intel Corporation. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just Copyright (c) 2022 Intel Corporation.?

Copy link
Contributor

Choose a reason for hiding this comment

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

done

GarveyJoe
GarveyJoe reviewed Nov 25, 2022
View reviewed changes
sycl/doc/extensions/proposed/sycl_ext_oneapi_annotated_ptr.asciidoc Outdated

// alignment in bytes and address bus width of the pointer specified using the
// properties 'alignment' and 'awidth'.
// a properties object is deducted from the list the of property values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// a properties object is deducted from the list the of property values
// a properties object is deduced from the list the of property values

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@gmlueck gmlueck mentioned this pull request Dec 5, 2022
Merged
@broxigarchen
Copy link
Contributor

Hi @steffenlarsen @Pennycook Please let me know your thoughts on this PR. Thanks!

Pennycook
Pennycook reviewed Dec 14, 2022
View reviewed changes
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

I don't want to block this. The only things I could spot were related to names.

sycl/doc/extensions/proposed/sycl_ext_intel_fpga_kernel_arg_properties.asciidoc
Comment on lines +215 to +224
template<read_write_mode_enum mode>
inline constexpr read_write_mode_key::value_t<mode>
read_write_mode;
inline constexpr read_write_mode_key::value_t<
read_write_mode_enum::read> read_write_mode_read;
inline constexpr read_write_mode_key::value_t<
read_write_mode_enum::write> read_write_mode_write;
inline constexpr read_write_mode_key::value_t<
read_write_mode_enum::read_write>
read_write_mode_readwrite;
Copy link
Contributor

Choose a reason for hiding this comment

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

read_write_mode_readwrite is a really bad name.

Would it make sense to split this out into separate read, write and read_write properties?

sycl/doc/extensions/proposed/sycl_ext_intel_fpga_kernel_arg_properties.asciidoc
inline constexpr wait_request_key::value_t<true>
wait_request_requested;
inline constexpr wait_request_key::value_t<false>
wait_request_not_requested;
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the name wait_request_not_requested really confusing, and I don't think this convenience type helps.

The second use of "requested" doesn't seem to be tied to the first use of "request", and the description of wait_request later in the document talks about whether the wait request signal is generated rather than requested. wait_request<true> is also shorter.

steffenlarsen
steffenlarsen approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

My concerns have been addressed, so LGTM! That said, I agree with @Pennycook on the property shorthand namings.

@pvchupin pvchupin merged commit b423e1d into intel:sycl Dec 15, 2022
againull pushed a commit that referenced this pull request Jan 12, 2023
@wangdi4
[SYCL] Implement annotated_arg headers (#6894)
1649fe4 
Add `annotated_arg` headers based on
[sycl_ext_oneapi_annotated_arg.asciidoc](https://github.com/intel/llvm/blob/32564a4fed2ec8bdae182123b0d6b4cbce1590f2/sycl/doc/extensions/proposed/sycl_ext_oneapi_annotated_arg.asciidoc)
in PR #5755

---------

Co-authored-by: Chen, Brox <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keryell keryell keryell left review comments

@gmlueck gmlueck gmlueck left review comments

@dm-vodopyanov dm-vodopyanov dm-vodopyanov left review comments

@broxigarchen broxigarchen broxigarchen left review comments

@GarveyJoe GarveyJoe GarveyJoe left review comments

@jessicadavies-intel jessicadavies-intel jessicadavies-intel left review comments

@Pennycook Pennycook Pennycook approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@rolandschulz rolandschulz Awaiting requested review from rolandschulz

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

Successfully merging this pull request may close these issues.
10 participants
@tiwaria1 @pvchupin @gmlueck @steffenlarsen @broxigarchen @Pennycook @keryell @dm-vodopyanov @GarveyJoe @jessicadavies-intel