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

[SYCL][Graph] Add exceptions on using spec constants, kernel bundles and reductions #290

Merged
merged 8 commits into from
Aug 10, 2023

Conversation

Bensuo
Copy link
Collaborator

@Bensuo Bensuo commented Aug 7, 2023

  • Add exceptions when using spec constants, kernel bundles and reductions
  • Unit tests for spec constants and kernel bundles.
  • Refactored handler throwing code to remove templates.
  • Moved and renamed unsupported features enum to graph header.

- Add exceptions when using spec constants and kernel bundles
- Unit tests for both of these.
- Refactored handler throwing code to remove templates.
- Moved and renamed unsupported features enum to graph header.
@Bensuo Bensuo added Graph Implementation Related to DPC++ implementation and testing cherry-pick labels Aug 7, 2023
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM, I'm hopeful that removing templates from handler::throwIfGraphAssociated will solve the Windows CI fail, but I can try verify that locally.

sycl/include/sycl/handler.hpp Outdated Show resolved Hide resolved
sycl/source/handler.cpp Outdated Show resolved Hide resolved
@EwanC
Copy link
Collaborator

EwanC commented Aug 7, 2023

I'm hopeful that removing templates from handler::throwIfGraphAssociated will solve the Windows CI fail, but I can try verify that locally.

Yup, this branch does fix the linking error on Windows

- Add exceptions on reduction use
- Updated reduction tests to be XFAIL
- Tweak exception message wording
@Bensuo Bensuo changed the title [SYCL][Graph] Add exceptions on using spec constants and kernel bundles [SYCL][Graph] Add exceptions on using spec constants, kernel bundles and reductions Aug 7, 2023
@Bensuo
Copy link
Collaborator Author

Bensuo commented Aug 7, 2023

@EwanC I've also pushed changes adding exceptions for Reductions here to make use of the refactored error handling code. Tagging for re-review, sorry!

@Bensuo Bensuo requested a review from EwanC August 7, 2023 15:43
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Reductions LGTM, minor comment at unittesting it

sycl/unittests/Extensions/CommandGraph.cpp Show resolved Hide resolved
Copy link
Collaborator

@mfrancepillois mfrancepillois left a comment

Choose a reason for hiding this comment

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

In a previous PR, I changed the parametrized throwIfGraphAssociated function into a template function to be in line with other throw functions used by handle (as suggest by @reble in his comment #276 (comment) ).
I understand that changing back the template function into a parametrized function fixes the Windows compilation issue, but we should be aware that less consistent with the other throw functions of this file (and it also may impact performances as it makes the inlining of this function less intuitive for the compiler?)

Copy link
Owner

@reble reble left a comment

Choose a reason for hiding this comment

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

LGTM

@Bensuo
Copy link
Collaborator Author

Bensuo commented Aug 10, 2023

In a previous PR, I changed the parametrized throwIfGraphAssociated function into a template function to be in line with other throw functions used by handle (as suggest by @reble in his comment #276 (comment) ). I understand that changing back the template function into a parametrized function fixes the Windows compilation issue, but we should be aware that less consistent with the other throw functions of this file (and it also may impact performances as it makes the inlining of this function less intuitive for the compiler?)

I've changed this back to a template but kept the definition in the header to avoid the template specializations 👍

Copy link
Collaborator

@mfrancepillois mfrancepillois left a comment

Choose a reason for hiding this comment

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

LGTM

@Bensuo Bensuo merged commit 04941fd into sycl-graph-develop Aug 10, 2023
@Bensuo Bensuo deleted the ben/spec-const-bundle-exception branch August 10, 2023 14:34
EwanC added a commit that referenced this pull request Aug 11, 2023
…and reductions (#290)

- Add exceptions when using spec constants, reductions and kernel bundles
- Unit tests added for these.
- Refactored handler throwing code to remove templates specializations.
- Moved and renamed unsupported features enum to graph header.
- Updated reduction tests to be XFAIL
- Tweak exception message wording
- Update ABI symbol tests.

---------

Co-authored-by: Ewan Crawford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick Graph Implementation Related to DPC++ implementation and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants