Skip to content
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] Implement OpenCL kernel function generation #249

Conversation

Fznamznon
Copy link
Contributor

@Fznamznon Fznamznon commented Jun 25, 2019

I'd like to review OpenCL kernel generation part before pushing it to reviews.llvm.org since it's very big and complicated change with it's own issues and limitations.
For example, our detecting of SYCL special objects, e.g. accessors/samplers seems a bit fragile.

@Fznamznon Fznamznon requested a review from bader June 25, 2019 13:53
@Fznamznon Fznamznon added the upstream This change is related to upstreaming SYCL support to llorg. label Jun 25, 2019
@Fznamznon
Copy link
Contributor Author

@bader , do we still need this PR?

@bader
Copy link
Contributor

bader commented Nov 14, 2019

@bader , do we still need this PR?

We need similar PR, but I think the right way to generate OpenCL kernels is to emit LLVM IR in CodeGen, rather than build an AST in Sema. If you agree, we should be refactor the patch....

@Fznamznon
Copy link
Contributor Author

@bader , do we still need this PR?

We need similar PR, but I think the right way to generate OpenCL kernels is to emit LLVM IR in CodeGen, rather than build an AST in Sema. If you agree, we should be refactor the patch....

If it's doable and will make our code better, then I'm ok with it. I think this PR can be closed then.

@Fznamznon Fznamznon closed this Nov 14, 2019
@bader
Copy link
Contributor

bader commented Nov 22, 2019

Let's rebase existing approach to https://reviews.llvm.org/D60455, upload to LLVM Phabricator and ask Clang devs for the feedback. Based on the feedback we decide whether it make sense to re-work the current approach.

@bader bader reopened this Nov 22, 2019
@bader bader assigned Fznamznon and unassigned bader Nov 22, 2019
@Fznamznon
Copy link
Contributor Author

@bader , do we need base OpenCL kernel generation on this patch - 194cc3a ?
I think if our current target is compile a simple SYCL sample now (for example https://github.com/codeplaysoftware/computecpp-sdk/blob/master/samples/accessors.cpp), we can simplify the first patch for OpenCL kernel generation by excluding support of some special cases like sampler or wrapped accessor.
It also can help with review and decision about approach reworking.
Please provide your opinion on this.

@bader
Copy link
Contributor

bader commented Nov 26, 2019

I agree that sampler support is probably not a requirement for the first iteration.
You can try drop wrapped accessor support too, if it's easier to implement.

@Fznamznon Fznamznon force-pushed the private/mpodchis/upstream_ocl_kernel_gen branch from 9165e60 to 06c7431 Compare December 3, 2019 14:30
clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved
clang/lib/Parse/ParseAST.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
// All special SYCL objects must have __init method
CXXMethodDecl *InitMethod = getInitMethod(CRD);
assert(InitMethod &&
"The accessor/sampler must have the __init method");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "sampler" from here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable.
"__init method is expected"

createSpecialSYCLObjParamDesc(Fld, ArgTy);
} else if (!ArgTy->isStandardLayoutType()) {

// TODO: Do we need diagnostics with the kernel gen patch?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bader , what do you think about adding diagnostics with OpenCL kernel generation patch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove and proceed with uploading the patch to reviews.llvm.org.

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp Outdated Show resolved Hide resolved
@Fznamznon Fznamznon force-pushed the private/mpodchis/upstream_ocl_kernel_gen branch from 06c7431 to 07554f8 Compare December 4, 2019 10:08
@Fznamznon
Copy link
Contributor Author

@bader , could you please take a look? I have a couple of questions here.
If everything is ok, I'll move it to LLORG review.

@Fznamznon Fznamznon changed the title [SYCL] Upstream OpenCL kernel generation [SYCL] Implement OpenCL kernel function generation Dec 4, 2019
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. As there is no way in OpenCL to pass
structures with pointers inside as kernel arguments, all memory objects
shared between host and device must be passed to the kernel as raw
pointers. SYCL also has a special mechanism for passing kernel arguments
from host to the device. In OpenCL kernel arguments are set by calling
`clSetKernelArg` function for each kernel argument, meanwhile in SYCL all the
kernel arguments are fields of "SYCL kernel function" which can be defined
as a lambda function or a named function object and passed as an argument
to SYCL function for invoking kernels (such as `parallel_for` or `single_task`).

To facilitate the mapping of SYCL kernel data members to OpenCL kernel
arguments and overcome OpenCL limitations we added the generation of an
OpenCL kernel function inside the compiler. An OpenCL kernel function
contains the body of the SYCL kernel function, receives OpenCL-like
parameters and additionally does some manipulation to initialize SYCL
kernel data members with these parameters. In some pseudo code the OpenCL
kernel function can look like this:

```
// SYCL kernel is defined in SYCL headers:
template <typename KernelName, typename KernelType/*, ...*/>
__attribute__((sycl_kernel)) void sycl_kernel_function(KernelType KernelFuncObj) {
  // ...
  KernelFuncObj();
}

// Generated OpenCL kernel function
__kernel KernelName(global int* a) {
  KernelType KernelFuncObj; // Actually kernel function object declaration
  // doesn't have a name in AST.
  // Let the kernel function object have one captured field - accessor A.
  // We need to init it with global pointer from arguments:
  KernelFuncObj.A.__init(a);
  // Body of the SYCL kernel from SYCL headers:
  {
    KernelFuncObj();
  }
}
```
OpenCL kernel function is generated by the compiler inside the Sema
using AST nodes.
@Fznamznon Fznamznon force-pushed the private/mpodchis/upstream_ocl_kernel_gen branch from 07554f8 to 02e8944 Compare December 4, 2019 15:22
@Fznamznon
Copy link
Contributor Author

@vladimirlaz
Copy link
Contributor

@Fznamznon , is this PR needed? Can we close it?

@bader
Copy link
Contributor

bader commented Jan 22, 2021

@Fznamznon , is this PR needed? Can we close it?

The patch is outdated. I rebased it in #3071, but it's quite different from what we have today in the sycl branch. The code has been significantly refactored by @erichkeane and @Fznamznon.
@Fznamznon, @erichkeane, do you have bandwidth to update this patch or 0a6590d (rebased on top of ToT + https://reviews.llvm.org/D89909).

@erichkeane
Copy link
Contributor

I don't.

@Fznamznon
Copy link
Contributor Author

@Fznamznon , is this PR needed? Can we close it?

The patch is outdated. I rebased it in #3071, but it's quite different from what we have today in the sycl branch. The code has been significantly refactored by @erichkeane and @Fznamznon.
@Fznamznon, @erichkeane, do you have bandwidth to update this patch or 0a6590d (rebased on top of ToT + https://reviews.llvm.org/D89909).

I don't.

@github-actions github-actions bot added the Stale label Feb 19, 2022
@bader bader closed this Feb 19, 2022
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
Enables tests that were marked with UNSUPPORTED: cuda, but are in fact passing.

Result of work on intel#249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale upstream This change is related to upstreaming SYCL support to llorg.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants