bader added a subscriber: ABataev. bader added inline comments.
================ Comment at: clang/docs/SYCLSupport.md:73 +the integration header is used (included) by the SYCL runtime implementation, so +the header must be available before the host compilation starts.* + ---------------- Naghasan wrote: > > First, it must be possible to use any host compiler > > I don't understand the link with the integration header. SYCL being > implementable as a library is a design principle of the specs but it doesn't > means the clang host compiler has to remain a vanilla C++ compiler. > > > information provided in the integration header is used (included) by the > > SYCL runtime implementation, so the header must be available before the > > host compilation starts > > Another approach to the integration header would be for clang as the host > compiler to generate the used type traits. > > First, it must be possible to use any host compiler > > I don't understand the link with the integration header. SYCL being > implementable as a library is a design principle of the specs but it doesn't > means the clang host compiler has to remain a vanilla C++ compiler. > > > information provided in the integration header is used (included) by the > > SYCL runtime implementation, so the header must be available before the > > host compilation starts > > Another approach to the integration header would be for clang as the host > compiler to generate the used type traits. If there are no objections from @keryell, I'd like to prototype this approach for SYCL first to make sure there are no blocking issues. This option seems to be worth to explore considering integration header approach disadvantages. ================ Comment at: clang/docs/SYCLSupport.md:123 +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. + ---------------- Naghasan wrote: > OpenMP offload uses a similar approach isn't it? Might be worth to describe > how the 2 relates to each other and where they diverge. Do you mean the approach OpenMP compiler uses to outline single-source code parts to offload? To be honest, I'm not sure... @ABataev, is there any description how OpenMP compiler outlines device code? https://clang.llvm.org/docs/OpenMPSupport.html doesn't provide much details unfortunately. ================ Comment at: clang/docs/SYCLSupport.md:130 +`accessor` classes. Raw pointers map to kernel parameters one-to-one without +additional transformations. `accessor` classes require additional processing as +The "device" implementation of this class contains pointers to the device memory ---------------- Naghasan wrote: > > Raw pointers map to kernel parameters one-to-one without additional > > transformations > > Is this true ? I thought they should be passed as "pointer to global memory" > (under the OpenCL or CUDA model). Good catch. I'll adjust the wording. ================ Comment at: clang/docs/SYCLSupport.md:132 +The "device" implementation of this class contains pointers to the device memory +as a class member. OpenCL doesn't allow passing structures with pointer type +members as kernel parameters. All memory objects shared between host and device ---------------- Naghasan wrote: > Won't be better to refer to SPIR or SPIR-V kernels rather than OpenCL ? > > Or even SPIR-like or SPIR-defacto to be even less normative. Is it okay if I refer to OpenCL SPIR-V environment specification: https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Env.html#_kernels? ================ Comment at: clang/docs/SYCLSupport.md:134 +members as kernel parameters. All memory objects shared between host and device +must be passed to the kernel as raw pointers. + ---------------- Naghasan wrote: > This is a bit ambiguous and leaves the impression you can't objects pass by > value. Agree. Removed. ================ Comment at: clang/docs/SYCLSupport.md:161 +// Generated kernel function (expressed in OpenCL-like pseudo-code) +__kernel KernelName(global int* a) { + KernelType KernelFuncObj; // Actually kernel function object declaration ---------------- Naghasan wrote: > This is missing the template instantiation that will eventually lead to > that lowering. > > I would also suggest to split code block in 2 as to mark what is in header > and source file (SYCL code) and what is compiler generated (that pseudo > OpenCL). > > Might be good to also mention the glue generated in the integration header as > this is what allows arguments to be set by the runtime (bridge between the > structure in C++ and the SPIR-like kernel arguments). > This is missing the template instantiation that will eventually lead to > that lowering. > I would also suggest to split code block in 2 as to mark what is in header > and source file (SYCL code) and what is compiler generated (that pseudo > OpenCL). Do you suggest to sketch an SYCL kernel invocation API usage example with the complete implementation of these methods to demonstrate the full stack? I was trying to avoid runtime/header part of implementation in this section and describe only API compiler relies on or provides for the runtime library. I think these details were in this document before, but moved to https://github.com/intel/llvm/blob/sycl/sycl/doc/KernelParameterPassing.md referenced below. > Might be good to also mention the glue generated in the integration header as > this is what allows arguments to be set by the runtime (bridge between the > structure in C++ and the SPIR-like kernel arguments). I've mentioned that, but I hope KernelParameterPassing.md document provides the rest of the details. Is that okay? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99190/new/ https://reviews.llvm.org/D99190 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
