-
Notifications
You must be signed in to change notification settings - Fork 731
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][ESIMD] Report an error when slm_init is called more than once in the kernel #12804
Conversation
unsigned Idx = 0; | ||
for (const Argument &Arg : F.args()) { | ||
if (Arg.getType()->isPointerTy()) { | ||
auto *KernelArgAccPtrs = F.getMetadata("kernel_arg_accessor_ptr"); |
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.
Can we send an email to the CFE/SYCL language team as a non-blocking follow-up to see if they have any ideas on improving this check by doing it somewhere else (Sema?). Relying on the metadata should work but it seems a bit indirect and possibly flaky, but I don't know how to do any better today.
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.
Will do, although it looks like a standard way to mark passing accessors to the kernel from FE
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 have couple of concerns in this patch: 1st - test is probably not valid (see other comment below),
2nd is related to a case with 1 kernel using slm_init
and 2nd kernel using local_accessor
.
Please add a test for this scenario.
// This test verifies call to slm_init from a function not marked as | ||
// always_inline triggers an error. |
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.
nit: the comment is not precise. It tests the case when a function is marked noinline
, not not marked as always_inline
.
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.
Fixed
How about using such algorithm?
There may be some extra data needed here to emit a meaningful/helpful error. It is more the idea above. |
I believe the current implementation is using more or less the same algorithm but in slightly different way: It uses a set to see if a kernel called slm_init one way or another I probably need to tweak it for multiple kernels in a module |
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.
Looks good. Thank you.
Just 1 minor comment - SPIRV_LOCAL_ACCESSOR_PREF is not needed after the recent logic simplification patch.
Co-authored-by: Vyacheslav Klochkov <[email protected]>
No description provided.