[SYCL] Add SYCL clang frontend doc #177
Conversation
There was a problem hiding this comment.
Thank you for adding some documentation.
Just a few typos.
@agozillon we need a native English speaker for the proofreading of the documentation.
|
|
||
| All SYCL memory objects shared between host and device (buffers/images, | ||
| these objects map to OpenCL buffers and images) must be accessed through special | ||
| `accessor` classes. The "device" side implementation of these classes contain pointers to the device memory. There is no |
There was a problem hiding this comment.
contains (+ end-of-line to split the too long line)
There was a problem hiding this comment.
I think in the context of this sentence contain is correct grammar
There was a problem hiding this comment.
There is no way in OpenCL -> As there is no way in OpenCL
I think this keeps the original intent of the sentence but flows better
There was a problem hiding this comment.
I rewrote it with addition that in OpenCL all memory objects shared between host and device must be passed to the kernel as raw pointers.
|
|
||
| // Kernel wrapper | ||
| __kernel wrapper(global int* a) { | ||
| lambda; // Actually lambda declaration doesn't have a name in AST |
There was a problem hiding this comment.
Lambda it's unnamed function object declared by lambda expression passed to parallel_for. I added type and comment with clarification.
|
|
||
| ```C++ | ||
| // SYCL kernel is defined in SYCL headers | ||
| __attribute__((sycl_kernel)) someSYCLKernel(lambda) { |
There was a problem hiding this comment.
What is the meaning of this "lambda"? Where is the type?
Do you mean something like this:
template <typename Lambda>
__attribute__((sycl_kernel)) someSYCLKernel(Lambda lambda) {
?
There was a problem hiding this comment.
Yes, I added type.
| device, if in OpenCL you need to call `clSetKernelArg`, in SYCL all | ||
| kernel arguments are captures/fields of lambda/functor which is passed to | ||
| `parallel_for` (in code snippet above one kernel argument - `accessor A`). | ||
| To map to OpenCL kernel arguments setting mechanism we added generation of |
There was a problem hiding this comment.
"To map to OpenCL kernel arguments setting mechanism we added generation of "kernel wrapper" function inside the compiler."
Sorry, I have a hard time understanding this sentence (perhaps that's just me though). Is it perhaps better worded to something like the following (without losing your original point):
"To facilitate the mapping of the captures/fields of lambdas/functors to OpenCL kernel arguments we added the generation of a "kernel wrapper" function inside the compiler."
There was a problem hiding this comment.
Actually kernel wrapper not only "To facilitate the mapping of the captures/fields of lambdas/functors to OpenCL kernel arguments" but also to overcome OpenCL limitations about pointers inside structures, so I rewrote using your comment and little addition.
|
Overall well written and I can understand it, I did add some suggested rewording and corrections which you can ignore/use/or cherry pick parts from. I think in most cases it will aid people unfamiliar with the codebase or SYCL, so hopefully it's helpful suggestions rather than irritating or too pedantic! Thanks for the great work. |
Thank you for your comments, It really helps make this document sound better, I appreciate it. |
|
This looks fantastic, just added a few final very minor comments that you can change if you feel like it. |
Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>
Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com> Co-Authored-By: Sergey Semenov <sergey.semenov@intel.com> Co-Authored-By: Alexey Bader <alexey.bader@intel.com>
Fznamznon
force-pushed
the
private/mpodchis/doc
branch
from
769edf3
to
9bce529
Compare
bader
dismissed
their stale review
Let's see if buildbot checks will stop if I dismiss the review.
|
LGTM, thank you for the great documentation addition! |
| The SYCL runtime library can't mark foo as "device" code - this is a compiler | ||
| job: to traverse all symbols accessible from kernel functions and add them to | ||
| the "device part" of the code marking them with the new SYCL device attribute. | ||
|
|
There was a problem hiding this comment.
I think we should clarify the requirements we put on the function, which SYCL runtime can mark with sycl_kernel attribute.
I can remember at least these off the top of my head:
- Must be template function with at least two template parameters.
- First parameter must represent "unique kernel name"
- Second parameter must be the function object type
- Must have only one function parameter.
- Parameter type must be second template parameter
template <typename KernelName, typename KernelType/*, ...*/>
__attribute__((sycl_kernel)) void sycl_kernel_function(KernelType KernelFuncObj) {
KernelFuncObj();
}Other requirements can be extracted from the SemaSYCL.cpp.
It's some informal API, which we should document somewhere.
Maybe it would be better to document in comments. What do you think?
There was a problem hiding this comment.
I think we can add some documentation to comments and this document both. But IMO requirements for function marked with sycl_kernel attribute aren't related to the "device code oulining" feature but related to generation of kernel wrapper from this function. So I suggest adding it to the "SYCL kernel function object (functor or lambda) lowering" section.
Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>
Fznamznon
force-pushed
the
private/mpodchis/doc
branch
from
9bce529
to
b9ee235
Compare
bader
dismissed
MrSidims’s stale review
Please, provide more information on what exact changes do you request.
bader
added
the
upstream
No description provided.