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

[Alexey Bader via Phabricator via cfe-commits]

bader marked 2 inline comments as done.
bader added inline comments.


================
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) {
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72857