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