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

Wed Apr 17 04:18:01 PDT 2019

Anastasia added a comment.

In D60455#1469150 <https://reviews.llvm.org/D60455#1469150>, @aaron.ballman wrote:

> In D60455#1468714 <https://reviews.llvm.org/D60455#1468714>, @bader wrote:
>
> > In D60455#1468386 <https://reviews.llvm.org/D60455#1468386>, @Fznamznon wrote:
> >
> > > > Ok, my question is whether you are planning to duplicate the same logic as for OpenCL kernel which doesn't really seem like an ideal design choice. Is this the only difference then we can simply add an extra check for SYCL compilation mode in this template handling case. The overall interaction between OpenCL and SYCL implementation is still a very big unknown to me so it's not very easy to judge about the implementations details...
> > >
> > > Of course, if nothing prevents us to re-use OpenCL kernel attribute for SYCL I assume it would be good idea. 
> > >  But I'm thinking about the situation with https://reviews.llvm.org/D60454 . If we re-use OpenCL kernel attributes - we affected by OpenCL-related changes and OpenCL-related changes shouldn't violate SYCL semantics. Will it be usable for SYCL/OpenCL clang developers? @bader , what do you think about it?
> >
> >
> > I also think it's worth trying. We should be able to cover "SYCL semantics" with LIT test to avoid regressions by OpenCL related changes. E.g. add a test case checking that -fsycl-is-device option disables restriction on applying `__kernel` to template functions.
> >  I want to confirm that everyone is okay to enable `__kernel` keyword for SYCL extension and cover SYCL use cases with additional regression tests. IIRC, on yesterday call, @keryell, said that having SYCL specific attributes useful for separation of concerns.
>
>
> I'm not comfortable with that decision unless the attribute semantics are sufficiently related to justify it. If we're just going to have a lot of `KernelAttr->isSYCL()` vs `KernelAttr->isOpenCL()` accessor calls, it may make more sense to use separate semantic attributes (even if they share spellings), though then I'd be curious how a user combines OpenCL and SYCL attributes.

I am not sure we need to add a keyword actually, the attribute can just be added in AST since it's not supposed to be used in the source code? My understanding of SYCL kernel is that it mainly matches OpenCL kernel functionality because the original intent of SYCL was to provide single source functionality on top of OpenCL. But I am not an expert in SYCL to confirm that. I think what we are missing currently is a thorough analysis/comparison between SYCL device mode and OpenCL kernel language mode to understand what's the best implementation strategy. That would apply to many other features: kernel function restrictions, address spaces, vectors, special types, etc. I still see no point in polluting our code base with extra code that just does the same thing. It will save us a lot of time to just work cooperatively on the same problem and even improve readability of the code. But of course this can only be done if there is no need to diverge the implementation significantly.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455