Skip to content

Conversation

@Pennycook
Copy link
Contributor

Brings LocalMemory extension in line with other extensions:

  • Written against SYCL 2020
  • Feature test macro
  • sycl::ext::oneapi namespace

This commit replaces the old SYCL 1.2.1 extension completely
in favor of a SYCL 2020 extension, because the SYCL 1.2.1 extension
has not yet been marked implemented.

Signed-off-by: John Pennycook [email protected]

Brings LocalMemory extension in line with other extensions:
- Written against SYCL 2020
- Feature test macro
- sycl::ext::oneapi namespace

This commit replaces the old SYCL 1.2.1 extension completely
in favor of a SYCL 2020 extension, because the SYCL 1.2.1 extension
has not yet been marked implemented.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook Pennycook added spec extension All issues/PRs related to extensions specifications Documentation Missing documentation for the code, compiler or runtime features, etc. labels Aug 12, 2021
@Pennycook Pennycook requested a review from a team as a code owner August 12, 2021 16:02
@Pennycook
Copy link
Contributor Author

Modernizing the documentation ahead of marking the extension "implemented" as discussed in #4323. While converting to the new extension format I noticed a few typos and copy-paste errors, which this commit also fixes.

@sergey-semenov: Tagging you so that you can double-check this matches the implementation.

@oleksandr-pavlyk
Copy link
Contributor

The example is most useful. It would be further useful if the document better explained how group_local_memory<int[64]>(item.get_group()) is different from group_local_memory_for_overwrite<int[64]>(item.get_group());

Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

NOTE: This document is better viewed when rendered as html with asciidoctor. GitHub does not render image icons.

I did not include this statement in some of the more recent spec's I've written. In my experience GitHub renders the asciidoctor sources pretty well, and I think we would want to encourage people to use the GitHub rendering. Asking people to clone the repo, install asciidoctor, and run it manually is a real pain. Do you really think we need this statement?

Built On: {docdate} +

I also did not include this line in my recent specs. When you use the GitHub rendering, this simply shows the current date, which is not useful. If you want the spec to contain a date, it would be better to hard-code here. Alternatively, it may be better to use use the revision number to identify revisions of the spec.

@bader bader requested review from gmlueck and mohammadfawaz August 18, 2021 19:33
mohammadfawaz
mohammadfawaz previously approved these changes Aug 18, 2021
@bader bader merged commit 6ed6565 into intel:sycl Aug 19, 2021
@Pennycook Pennycook deleted the local-memory-2020 branch November 11, 2021 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Documentation Missing documentation for the code, compiler or runtime features, etc. spec extension All issues/PRs related to extensions specifications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants