[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Ronan Keryell via Phabricator via cfe-commits]

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