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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
