Skip to content

[SYCL] Fix host device local accessor alignment #5554

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

Merged
merged 3 commits into from
Mar 1, 2022

Conversation

npmiller
Copy link
Contributor

@npmiller npmiller commented Feb 11, 2022

Local kernel arguments must be aligned to the type size, simply using std::vector<char> doesn't always provide the correct alignment. So this patch adds extra padding to the vector and ensures that the pointer returned for the accessor is actually aligned to the type size.

This issue was exposed by: intel/llvm-test-suite#608, which was a follow up to fixing local accessor alignment for the CUDA plugin.

Local kernel arguments must be aligned to the type size, so simply using
`std::vector<char>` doesn't always provide the correct alignment.

Instead using the `aligned_allocator` makes sure the allocation is
aligned maximum possible required alignment.
@npmiller npmiller requested a review from a team as a code owner February 11, 2022 16:52
@smaslov-intel
Copy link
Contributor

However this patch is breaking ABI (and currently fails the ABI tests), so some feedback on whether this is acceptable of it there is another way to approach it which wouldn't break ABI would be appreciated.

I don't see how an alignment could be forced without changing the ABI, and the ABI breaking change is not allowed until major update. @intel/llvm-reviewers-runtime , any idea?

@romanovvlad
Copy link
Contributor

I don't see how an alignment could be forced without changing the ABI, and the ABI breaking change is not allowed until major update. https://github.com/orgs/intel/teams/llvm-reviewers-runtime , any idea?

Can we modify LocalAccessorImplHost:LocalAccessorImplHost to allocate more memory(+ElemSize) and modify LocalAccessorBaseHost::getPtr to adjust the pointer so it's aligned enough for ElemSize?

@npmiller
Copy link
Contributor Author

Good shout @romanovvlad , updated as suggested, so it should now fix the alignment without breaking the ABI.

Not exactly related to this patch but I'm a little confused about the void* getPtr() const overloads, were these meant to be const void* getPtr() instead? There are two of them, one that I modified here and one in another class, and both are/were doing a const_cast<void *> on char* which doesn't change the constness.

@alexbatashev
Copy link
Contributor

/summary:run

@npmiller
Copy link
Contributor Author

ping @smaslov-intel @romanovvlad

@bader
Copy link
Contributor

bader commented Mar 1, 2022

@npmiller, could you update the PR description, so I can use it as a commit message, please?

@npmiller
Copy link
Contributor Author

npmiller commented Mar 1, 2022

PR description updated to reflect the current patch

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.

5 participants