[PATCH] D72857: [SYCL] Driver option to enable SYCL mode and select SYCL version

Tue Feb 18 01:46:50 PST 2020

Naghasan added inline comments.

================
Comment at: clang/include/clang/Basic/LangOptions.def:206
 LANGOPT(OpenCLCPlusPlusVersion     , 32, 0, "C++ for OpenCL version")
+ENUM_LANGOPT(SYCLVersion, SYCLVersionList, 4, SYCLVersionList::undefined, "Version of the SYCL standard used")
 LANGOPT(NativeHalfType    , 1, 0, "Native half type support")
----------------
Ruyk wrote:
> bader wrote:
> > All other language options controlling standard versions are added as "LANGOPT" i.e. `int`. Why SYCLVersion is different?
> > @Ruyk, do you think we should convert other options (e.g. `OpenCL`) to enums as well?
> Thats a good point. I don't see strong reasons for the enum, basically I read the comment in https://code.woboq.org/llvm/clang/include/clang/Basic/LangOptions.def.html#22
> 
> 
> ```
> // ENUM_LANGOPT: for options that have enumeration, rather than unsigned, type.
> ```
> 
> And since there is a known set of SYCL specifications, made more sense to enumerate it.
> It also simplifies writing variants to 1.2.1 (e.g. 1.2.1-oneapi) in the code since then you can add another entry to the enum.
> 
> But no strong feelings, so feel free to change it.
> 
`int` allows the use of relational operators which should ease version managements.

As `SYCLVersionList` is a strongly typed enum so this may not be the best, and as the SYCL version are now meant to be a year `int` should do just fine.

================
Comment at: clang/include/clang/Driver/Options.td:3401
+def sycl_std_EQ : Joined<["-"], "sycl-std=">, Group<sycl_Group>, Flags<[CC1Option]>,
+  HelpText<"SYCL language standard to compile for.">, Values<"1.2.1">;

----------------
Ruyk wrote:
> bader wrote:
> > What do you think we integrate sycl versions to existing clang options controlling language version: `-std`.
> > As far as I can see it's used for all the C/C+ extensions like OpenMP/OpenCL/CUDA/HIP/ObjC.
> > 
> > If I understand correctly clang supports `-cl-std` only because it's required by OpenCL standard. Similar option (i.e. `-sycl-std`) is not required by the SYCL specification, so using `-std` is more aligned with existing clang design.
> > 
> > See clang/include/clang/Basic/LangStandard.h and clang/include/clang/Basic/LangStandards.def.
> In the case of SYCL, you may want to compile your code with C++17 and SYCL 2015, in which case you need both -std=c++17 and -sycl=sycl-2015 . 
> SYCL specification mandates a minimum C++ version but users can write code on newer versions as long as the code in the kernel scope is still valid.
+1 on this, ComputeCpp used to mix-up both and this proved to be complex to manage. It also integrates better with build systems.

================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2548
+  Opts.SYCL = Args.hasFlag(options::OPT_fsycl, options::OPT_fno_sycl, false);
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+  if (Opts.SYCL || Opts.SYCLIsDevice) {
----------------
bader wrote:
> ABataev wrote:
> > bader wrote:
> > > ABataev wrote:
> > > > bader wrote:
> > > > > ABataev wrote:
> > > > > > bader wrote:
> > > > > > > ABataev wrote:
> > > > > > > > bader wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > This option also must be controlled by `-fsycl`:
> > > > > > > > > > ```
> > > > > > > > > > Opts.SYCLIsDevice =  Opts.SYCL && Args.hasArg(options::OPT_fsycl_is_device);
> > > > > > > > > > 
> > > > > > > > > > ```
> > > > > > > > > Does it really has to? This logic is already present in the driver and it makes front-end tests verbose `%clang_cc1 -fsycl -fsycl-is-device`.
> > > > > > > > > Can `-fsycl-is-device` imply `-fsycl`?
> > > > > > > > > Looking how CUDA/OpenMP options are handled, not all of them are processed using this pattern.
> > > > > > > > In general, this is how we handle it in OpenMP. Cuda works differently, because it has its own kind of files (.cu) and Cuda is triggered by the language switch (-x cu). Seems to me, you're using something close to OpenMP model, no? Or do you want to define your own language kind just like Cuda?
> > > > > > > I applied you suggest, although I don't fully understand the need of using two options instead of two. I would prefer having following code:
> > > > > > > ```
> > > > > > > Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
> > > > > > > Opts.SYCL = Args.hasArg(options::OPT_fsycl) || Opts.SYCLIsDevice; // -fsycl-is-device enable SYCL mode as well
> > > > > > > ```
> > > > > > I'm not quite familiar with SYCL model, maybe this the right approach. You'd better try to provide more details. Are there any differences between just SYCL and SYCL-device compilation modes? How do you see the compilation sequence in general? At first you're going to compile the host version of the code, then the device? OR something different?
> > > > > I think SYCL model is quite similar to OpenMP model. One significant difference might be that to implement standard SYCL functionality we don't need any modifications for the host compiler. AFAIK, OpenMP compiler has to support OpenMP pragmas. 
> > > > > We have a few attributes for Intel FPGA devices, which we can't pre-process with `__SYCL_DEVICE_ONLY__` macro and we have added "SYCL-host" mode to suppress compiler warnings about attributes ignored on the host. I think there might be other options how this can be achieved w/o adding new compilation mode and use regular C++ front-end as SYCL host compiler.
> > > > > I think there is a difference between SYCL and SYCL-device modes, but it's mostly changes the compilation workflow in the driver, but not in the front-end. In SYCL-device mode, driver invokes only one front-end instance to generate offload code. In SYCL mode, driver invokes multiple front-end instances: one in SYCL-device mode and one in regular C++ mode (to be accurate we use SYCL-host mode, but as I mentioned above I don't think it really needed).
> > > > > I hope it makes it clear now. Let me know if you have any other questions.
> > > > > 
> > > > > Do I understand it correctly that OpenMP option enables OpenMP mode, which is equivalent of "SYCL-host" mode and enabling both OpenMP and OpenMPIsDevice is required for enabling OpenMP mode, which is similar to SYCL-device mode?
> > > > > If so, can we assume that OpenMPIsDevice implies that OpenMP option is also set (implicitly)?
> > > > > Do I understand it correctly that OpenMP option enables OpenMP mode, which is equivalent of "SYCL-host" mode and enabling both OpenMP and OpenMPIsDevice is required for enabling OpenMP mode, which is similar to SYCL-device mode?
> > > > 
> > > > Well, for driver you need to pass `-fopenmp -fopenmp-target=<list_of_targets>` to enable the compilation with offloading support. In the frontend the host part is compiled with `-fopenmp` only (+ aux-triple, probably), for devices - `-fopenmp -fopenmp-is-device`. Without `-fopenmp` `-fopenmp-is-device` is just ignored.
> > > What is the reason to require the driver to pass both options for the devices? It sounds like `-fopenmp-is-device` should be enough to differentiate from the host part (`-fopenmp` only). Right?
> > We treat a little bit differently. `-fopenmp` turns support for OpenMP, while `-fopenmp-is-device` turns processing for device compilation.
> I'm okay to use OpenMP way to treat front-end options as they impact only clang developers (more verbose LIT test + a little bit more logic in the code), but I'd like to keep driver options handling simpler for clang users.
> We can discuss this later, when `-fsycl-device-only` driver option is added.
> 
> Do you have any other comments to this change?
> 
> BTW, thanks a lot for take your time to review SYCL patches - it's highly appreciated!
```
We treat a little bit differently. -fopenmp turns support for OpenMP, while -fopenmp-is-device turns processing for device compilation.
```

Seems to me that SYCL is kind of an hybrid here.

@bader do you plan on raising kernel specific diagnostics even on the host side ? In this case reusing this model will fully make sense.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72857