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