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

Mariya Podchishchaeva via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 23 01:08:49 PDT 2019


Fznamznon added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1017
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}
----------------
bader wrote:
> Anastasia wrote:
> > Ok, I thought the earlier request was not to add undocumented attributes with the spelling?
> > 
> > Also did `__kernel` attribute not work at the end?
> > 
> > I can't quite get where the current disconnect comes from but I find it extremely unhelpful.
> Hi @Anastasia, let me try to help.
> 
> > Ok, I thought the earlier request was not to add undocumented attributes with the spelling?
> 
> Right. @Fznamznon, could you document `sycl_kernel` attribute, please?
> 
> > Also did __kernel attribute not work at the end?
> 
> Maria left a comment with the summary of our experiment: https://reviews.llvm.org/D60455#1472705. There is a link to pull request, where @keryell and @agozillon expressed preference to have separate SYCL attributes. Let me copy their feedback here:
> 
> @keryell :
> 
> > Thank you for the experiment.
> > That looks like a straight forward change.
> > The interesting part is that it does not expose any advantage from reusing OpenCL __kernel marker.... So I am not more convinced that it is the way to go, because we would use any other keyword or attribute and it would be the same...
> > 
> 
> @agozillon :
> 
> > Just my two cents, I think a separation of concerns and having separate attributes will simplify things long-term.
> > 
> > While possibly similar just now, the SYCL specification is evolving and may end up targeting more than just OpenCL. So the semantics of the attributes may end up being quite different, even if at the moment the SYCL attribute is there mostly just to mark something for outlining.
> > 
> > If it doesn't then the case for refactoring and merging them in a future patch could be brought up again.
> 
> To summarize: we don't have good arguments to justify re-use of OpenCL `__kernel` keyword for SYCL mode requested by @aaron.ballman here https://reviews.llvm.org/D60455#1469150.
> 
> > I can't quite get where the current disconnect comes from but I find it extremely unhelpful.
> 
> Let me know how I can help here.
> 
> Additional note. I've submitted initial version of SYCL compiler design document to the GItHub: https://github.com/intel/llvm/blob/sycl/sycl/doc/SYCL_compiler_and_runtime_design.md. Please, take a look and let me know if you have questions.
>> Ok, I thought the earlier request was not to add undocumented attributes with the spelling?
> Right. @Fznamznon, could you document sycl_kernel attribute, please?

Do we really need add documentation for attribute which is not presented in SYCL spec and used for internal implementation details only because it has spelling?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455





More information about the cfe-commits mailing list