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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
