keryell accepted this revision.
keryell added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1000
+def SYCLDevice : InheritableAttr {
+ let Spellings = [GNU<"sycl_device">];
+ let Subjects = SubjectList<[Function, Var]>;
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > keryell wrote:
> > > Fznamznon wrote:
> > > > aaron.ballman wrote:
> > > > > Is there a reason to not also introduce a C++11 and C2x style
> > > > > spelling in the `clang` namespace? e.g., `[[clang::sycl_device]]`
> > > > I don't think that it makes sense because these attributes not for
> > > > public consumption. These attributes is needed to separate code which
> > > > is supposed to be offloaded from regular host code. I think SYCLDevice
> > > > attribute actually doesn't need a spelling because it will be added
> > > > only implicitly by compiler.
> > >
> > > If we go towards this direction, `[[clang::sycl::device]]` or
> > > `[[clang::sycl::kernel]]` look more compatible with the concept of name
> > > space.
> > > While not a public interface, if we have a kind of "standard" outlining
> > > in Clang/LLVM, some people might want to use it in some other contexts
> > > too.
> > If these are only being added implicitly by the compiler, then they should
> > not be given any `Spelling`. See `AlignMac68k` for an example.
> I'm still confused -- are these created implicitly or are they spelled out by
> the user explicitly? Right now, it looks like they're spelled out explicitly,
> but I was under the impression they are only intended to be created
> implicitly by the compiler.
>
> If they are expected to be explicitly specified by the user, the spelling
> should be using `Clang<>` instead of using `GNU<>`, `C2x<>`, and `CXX11<>`
> explicitly.
>
> > If we go towards this direction, [[clang::sycl::device]] or
> > [[clang::sycl::kernel]] look more compatible with the concept of name space.
>
> Attribute namespaces do not work that way. There is the vendor namespace and
> then the attribute name.
> Attribute namespaces do not work that way. There is the vendor namespace and
> then the attribute name.
After diving into "9.11.1 Attribute syntax and semantics [dcl.attr.grammar]" of
the latest C++ draft standard, it looks you are right... There is only 1 level
of `::` allowed. :-(
================
Comment at: clang/include/clang/Basic/AttrDocs.td:286
+help.
+ }];
+}
----------------
aaron.ballman wrote:
> I'm still not entirely certain how I would know what to mark and how. From
> the description, it sounds like whoever authors `parallel_for` needs to do
> this marking, or it somehow happens automatically?
>
> (I'll do another editorial pass once I understand the intended behavior a bit
> better -- I expect there will be a few more wording issues to address.)
In normal SYCL it happens automatically.
In the compiler unit-tests it is done manually to exercise the framework.
I am the one who suggested that in some other contexts, it could be used
manually for some special purpose like using some weird hardware, but I do not
want to derail the main review with this.
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
