[SYCL] Implement a builtin to mark a sycl kernel #3894
Conversation
The unique-stable-name constraint that you can't look up the name in a constant expression before instantiating the kernel is causing issues. This provides a constexpr builtin so that you can mark the kernel without having to instantiate the kernel.
|
This is an alternative solution to #3886 I think THIS fixes our problem, but I'm not positive it is what we want. Do we ever try to instantiate a KernelInfo on something that is NOT a kernel? @romanovvlad ? I also have a 'TODO' in here whether we need to do something to force the instantiation though. Otherwise, I want to make sure that this passes validation. I still have to spend some time getting this ready for commit (writing tests, perhaps a slight refactor) if we decide this is worth doing. |
|
Note the clang-format complaint is something I'm not willing to do. I'm not going to re-format the entirety of this comment that community has aligned intentionally. |
|
/summary:run |
|
Looks like WIndows likes it! @romanovvlad and @AlexeySachkov WDYT? |
erichkeane
requested review from
elizabethandrews,
premanandrao and
a team
as code owners
|
Ok, this should be functional now... hopefully CI stops being broken long enough to validate this :) EDIT: I'm STILL ignoring that clang-format, that is a much worse fix. |
Agreed, ignore clang-format in this case. |
|
/summary:run |
| @@ -187,6 +187,7 @@ class accessor { | |||
| template <int dimensions, access::mode accessmode, access::target accesstarget> | |||
| struct opencl_image_type; | |||
|
|
|||
| #ifdef __SYCL_DEVICE_ONLY__ | |||
There was a problem hiding this comment.
Note the CFE only provides these types in device mode, so this change lets us uses this in header properly in host mode.
We do not instantiate a |
Ok, thanks! As long as you're not doing so intentionally (that is, using it to test whether something is a kernel or not) than we're fine. All that will happen is that the unique-stable-name result will not be the name a of a kernel if you do (AND it likely messes up the name of other kernels around it). |
|
So I think we've got all the questions answered, so @AaronBallman and @elizabethandrews : are we happy with the CFE changes? Can someone from the @intel/llvm-reviewers-runtime take a look to make sure the change is acceptable? |
Co-authored-by: Alexey Bader <[email protected]>
|
@bader: The clang-format change is one we don't want to make, so this is ready! |
|
/summary:run |
bader
changed the title
The unique-stable-name constraint that you can't look up the name in a
constant expression before instantiating the kernel is causing issues.
This provides a constexpr builtin so that you can mark the kernel
without having to instantiate the kernel.