-
Notifications
You must be signed in to change notification settings - Fork 802
[ESIMD] SYCL device function cannot be called from ESIMD context #3511
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
Conversation
There are two subsequent changes I'm planning to make: 1. Mark all the ESIMD intrinsics with SYCL_ESIMD_FUNCTION. This change will change many lines, so I will submit it as a separate NFC patch. 2. Update the documentation to reflect this change.
| #define SYCL_ESIMD_KERNEL | ||
| #define SYCL_ESIMD_FUNCTION | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is needed because SYCL_ESIMD_FUNCTION is used in the headers included below.
Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
AaronBallman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
|
@erichkeane, @kbobrovs, @premanandrao, @elizabethandrews, please review. |
AaronBallman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from a missing line ending.
Added a new line
|
@kbobrovs, @premanandrao, @elizabethandrews, please review. |
AaronBallman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| template <typename T, int N, int M, int VStride, int Width, int Stride, | ||
| int ParentWidth = 0> | ||
| SYCL_EXTERNAL __SIGD::vector_type_t<T, M> | ||
| SYCL_EXTERNAL SYCL_ESIMD_FUNCTION __SIGD::vector_type_t<T, M> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the suggested way to add function attributes is after the last ')' - @erichkeane, is it correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that adds the attribute to the function type and not the function declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Aaron said! For an attribute to apply to the function declaration it needs to go on the left. See SYCL_EXTERNAL, which is a function declaration attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. We'll need to refactor examples and tests.
What would be the right place for the attribute to apply to a lambda?
single_task<class test>([](item<1> i) {}); ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a lambda, the only place you can currently put an attribute is in the type position (this was resolved for C++23 but SYCL can't require use of that yet, for obvious reasons). So:
single_task<class test>([](item<1> i) [[attr]] {});
is the only place for it to go on a lambda for the moment.
I will update the documentation in a subsequent patch.