[PATCH] D99190: WIP: [SYCL] Add design document for SYCL mode

[Victor Lomuller via Phabricator via cfe-commits]

Naghasan added inline comments.


================
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.
+
----------------
ABataev wrote:
> bader wrote:
> > 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.
> I don't think we have anything like this. Moreover, currently, there are 2 different models, one with outlining by the frontend and another one with the outlining by the LLVM.
I mentioned that as I know there is some support for CUDA and the clang driver openmp offload works with multiple frontend passes.
If the model(s) is too different then there is no point going further here. 

> Moreover, currently, there are 2 different models, one with outlining by the frontend and another one with the outlining by the LLVM.

I do recall some brief conversations about that. Are they meant to work in pair or one aims to replace the other ?


================
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
----------------
bader wrote:
> 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?
> Do you suggest to sketch an SYCL kernel invocation API usage example with the complete implementation of these methods to demonstrate the full stack?

Not the full stack, I agree that would be too much. Just the bare minimal that can show where the `global int* a` comes from and help connecting the dots. I agree details should be kept in the kernel param passing document.

So maybe consider adding something of those lines:

```
class MyObj {
  accessor<int> _ptr; // accessor<int> contains a pointer to the global address space.
public:

  void operator()(); // Body of the kernel
};

[...]
MyObj Obj{/*some init*/};
sycl_kernel_function<MyObj>(Obj);
```

This shows the instanciation and one can make the relation between this and the pseudo OpenCL kernel.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99190/new/

https://reviews.llvm.org/D99190