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

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1017
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}
----------------
Anastasia wrote:
> Undocumented -> SYCLKernelDocs
Oh, Thank you for that!


================
Comment at: clang/lib/Parse/ParseAST.cpp:171
 
+  if (S.getLangOpts().SYCLIsDevice) {
+    for (Decl *D : S.SyclDeviceFuncs()) {
----------------
Anastasia wrote:
> Do you also need to prevent generation of non-device functions somehow?
I think It's already prevented by change to CodeGenModule.cpp in this patch. CodeGen just ignores declarations without SYCL device attribute now.


================
Comment at: clang/lib/Sema/SemaSYCL.cpp:23
+
+  bool VisitCallExpr(CallExpr *e) {
+    if (FunctionDecl *Callee = e->getDirectCallee()) {
----------------
Anastasia wrote:
> 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...
> 
> 
Oh, sorry, I missed this comment when I updated patch last time.
Could you please advise in which form I can document it?


================
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
----------------
Anastasia wrote:
> 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.
I think this chunk of code seems big because of big repeated comment.


================
Comment at: clang/test/CodeGenSYCL/device-functions.cpp:24
+}
+// CHECK: define spir_func void @{{.*}}foo
+// CHECK: define linkonce_odr spir_func i32 @{{.*}}bar
----------------
Anastasia wrote:
> I can't see where the SPIR calling convention is currently set for SYCL?
If I understand correct it's set automatically on AST level because we use SPIR-based triple for device code. Only in case of C++ methods clang doesn't set SPIR calling convention. We did a modification in our codebase to get SPIR calling convention for C++ methods too (available [[ https://github.com/intel/llvm/blob/c13cb8df84418cb5d682f3bbee89090ebb0d00be/clang/lib/AST/ASTContext.cpp#L10015 | here ]] ) 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455