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

Add Alignment Imposition #512

Merged

Conversation

AD2605
Copy link
Contributor

@AD2605 AD2605 commented Dec 1, 2023

Imposes an alignment requirements on the vectors passed to SYCL' vector class' load/store method.
Fixes the issue described in #508

@CLAassistant
Copy link

CLAassistant commented Dec 1, 2023

CLA assistant check
All committers have signed the CLA.

@gmlueck
Copy link
Contributor

gmlueck commented Dec 14, 2023

@AD2605: I think we are waiting for you to update this PR.

@gmlueck gmlueck added Agenda To be discussed during a SYCL committee meeting and removed Agenda To be discussed during a SYCL committee meeting labels Dec 14, 2023
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.

The addendum sounds good, but I am still trying to understand the original sentence.

@AD2605
Copy link
Contributor Author

AD2605 commented Dec 14, 2023

@keryell do you mean the original sentence before the review comment this PR was proposing to add ?
Or the motivation behind this PR ?

@gmlueck
Copy link
Contributor

gmlueck commented Dec 14, 2023

@AD2605 will improve first sentence in description also.

@AD2605
Copy link
Contributor Author

AD2605 commented Jan 3, 2024

@gmlueck @keryell updated the original description of the load/store methods.
Lemme know if it's clear and concise, and if any further change might be required

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.

Thanks for your persistence. This looks good!

@gmlueck
Copy link
Contributor

gmlueck commented Jan 4, 2024

BTW, feel free to add yourself to the list of Contributors in "adoc/acknowledgements.adoc".

@AD2605
Copy link
Contributor Author

AD2605 commented Jan 4, 2024

Done

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.

Almost there!

@@ -17230,15 +17230,15 @@ a@
template <access::address_space AddressSpace, access::decorated IsDecorated>
void load(size_t offset, multi_ptr<const DataT, AddressSpace, IsDecorated> ptr)
----
a@ Loads the values at the address of [code]#ptr# offset in elements of type [code]#DataT# by [code]#NumElements * offset#, into the components of this SYCL [code]#vec#.
a@ Loads [code]#NumElements# elements into the components of this SYCL [code]#vec#. These elements are loaded from consecutive addresses, where the starting address is computed by adding [code]#offset * NumElements * sizeof(DataT)# bytes to the address specified by the [code]#ptr#. The ptr must be aligned to [code]#alignof(DataT)#.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
a@ Loads [code]#NumElements# elements into the components of this SYCL [code]#vec#. These elements are loaded from consecutive addresses, where the starting address is computed by adding [code]#offset * NumElements * sizeof(DataT)# bytes to the address specified by the [code]#ptr#. The ptr must be aligned to [code]#alignof(DataT)#.
a@ Loads [code]#NumElements# elements into the components of this SYCL [code]#vec#. These elements are loaded from consecutive addresses, where the starting address is computed by adding [code]#offset * NumElements * sizeof(DataT)# bytes to the address specified by the [code]#ptr#. The [code]#ptr# must be aligned to [code]#alignof(DataT)#.

Feel free to shorten the line.

Copy link
Contributor Author

@AD2605 AD2605 Jan 11, 2024

Choose a reason for hiding this comment

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

Thanks, added ptr inside the code section (conversation got automatically resolved as I committed the suggestion)

Co-authored-by: Ronan Keryell <ronan.keryell@amd.com>
@AD2605 AD2605 requested a review from keryell January 11, 2024 16:14
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!

@tomdeakin
Copy link
Contributor

WG approved, pleased merge

@gmlueck gmlueck merged commit f397f88 into KhronosGroup:SYCL-2020/master Jan 11, 2024
2 checks passed
keryell pushed a commit that referenced this pull request Sep 10, 2024
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.

7 participants