Skip to content

[SYCL] Move accessor_impl to source directory #6698

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 6 commits into from
Sep 13, 2022

Conversation

romanovvlad
Copy link
Contributor

@romanovvlad romanovvlad commented Sep 5, 2022

Moving impl part of host accessor implementation to avoid exposing
implementation details in the headers. This allows for more changes
in accessor without breaking ABI.

Also updated the gdb xmethods since it was relying on the
impl details which are not available for gdb(unless libsycl.so is
built with debug symbols) anymore. Instead of accessing members of
impl directly gdb printers now accessing helper methods. To prevent
compiler discarding these methods there are dummy references which
are active when NDEBUG is not defined.

Moving impl part of host accessor implementation to avoid exposing
implementation details in the headers. This allows for more changes
in accessor without breaking ABI.

Also updated the gdb pretty-printer since it was relying on the
impl details which are not available for gdb(unless libsycl.so is
built with debug symbols) anymore. Instead of accessing members of
impl directly gdb printers now accessing helper methods. To prevent
compiler discarding these methods there are dummy references which
are active when NDEBUG is not defined.
@romanovvlad romanovvlad marked this pull request as ready for review September 7, 2022 08:38
@romanovvlad romanovvlad requested review from a team as code owners September 7, 2022 08:38
v-klochkov
v-klochkov previously approved these changes Sep 7, 2022
Copy link
Contributor

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

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

Ok for ESIMD.

@romanovvlad
Copy link
Contributor Author

@steffenlarsen Could you please take a look?

steffenlarsen
steffenlarsen previously approved these changes Sep 8, 2022
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.

LGTM! 🚀

barisaktemur
barisaktemur previously approved these changes Sep 12, 2022
Copy link

@barisaktemur barisaktemur left a comment

Choose a reason for hiding this comment

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

Besides the code-reuse comment I made previously and the commit message suggestion below, the sycl/gdb/libsycl.so-gdb.py changes look fine to me.

@romanovvlad
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1252

@romanovvlad
Copy link
Contributor Author

Trying to fix unrelated fail here: intel/llvm-test-suite#1254

@romanovvlad romanovvlad merged commit 39eb5ed into intel:sycl Sep 13, 2022
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.

4 participants