[PATCH] D60455: [SYCL] Implement SYCL device code outlining

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1017
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}
----------------
Undocumented -> SYCLKernelDocs


================
Comment at: clang/include/clang/Basic/AttrDocs.td:269
+
+  using namespace cl::sycl;
+  queue Q;
----------------
The example doesn't demonstrate the use of the attribute.

It explains how it is used by the toolchain only!

May be @aaron.ballman can help here as I am not sure what the format should be.


================
Comment at: clang/lib/Parse/ParseAST.cpp:171
 
+  if (S.getLangOpts().SYCLIsDevice) {
+    for (Decl *D : S.SyclDeviceFuncs()) {
----------------
Do you also need to prevent generation of non-device functions somehow?


================
Comment at: clang/lib/Sema/SemaSYCL.cpp:23
+
+  bool VisitCallExpr(CallExpr *e) {
+    if (FunctionDecl *Callee = e->getDirectCallee()) {
----------------
Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > This is probably not something we can change at this point but I wish we could avoid complexities like this. :(
> > > 
> > > I think this is also preventing traditional linking of translation units. That is somewhat unfortunate.
> > > 
> > > It is good direction however to keep this logic in a separate dedicated compilation unit.
> > > 
> > > I would suggest to document it a bit more including any current limitations/assumption that you can mark under FIXME i.e. does your code handle lambdas yet, what if lambdas are used in function parameters, etc...
> > > I think this is also preventing traditional linking of translation units.
> > 
> > Could you elaborate more on this topic, please?
> > What do you mean by "traditional linking of translation units" and what exactly "is preventing" it?
> > Do you compare with the linking of regular C++ code (i.e. which do not split into host and device code)?
> > If so, SYCL is different from this model and more similar to CUDA/OpenMP models, which also skip "linking" of irrelevant part (e.g. host code is not linked by the device compiler).
> > Mariya added Justin (@jlebar) and Alexey (@ABataev), who work on single-source programming models to make them aware and provide feedback if any.
> Yes indeed, I mean linking of modules in C/C++ even though it doesn't necessarily mean linking of object files. So you don't plan to support `SYCL_EXTERNAL` in clang?
> 
> In CUDA the functions executed on device are annotated manually using `__device__` hence separate translation units can specify external device function... although I don't know if CUDA implementation in clang support this.
> 
> I guess OpenMP is allowed to fall back to run on host?
Ping!



> I would suggest to document it a bit more including any current limitations/assumption that you can mark under FIXME i.e. does your code handle lambdas yet, what if lambdas are used in function parameters, etc...




================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5520
                                             DefinitionRequired, true);
-              if (CurFD->isDefined())
+              if (CurFD->isDefined()) {
+                // Because all SYCL kernel functions are template functions - they
----------------
May be this should go into a helper function as it seems to be now a bigger chunk of code that is repeated?

Although, I am not very familiar with this code. You can try to get someone to review who has contributed to this more recently.


================
Comment at: clang/test/CodeGenSYCL/device-functions.cpp:24
+}
+// CHECK: define spir_func void @{{.*}}foo
+// CHECK: define linkonce_odr spir_func i32 @{{.*}}bar
----------------
I can't see where the SPIR calling convention is currently set for SYCL?


================
Comment at: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp:3
+// Now pretend that we're compiling regular C++ file without SYCL mode enabled.
+// There should be warnings.
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -x c++ %s
----------------
I don't think this comment is necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455