bader marked 3 inline comments as done. bader 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") ---------------- Naghasan wrote: > 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. Okay. I'll change the type from enum to int. ================ 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">; ---------------- Naghasan wrote: > 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. Okay. I'll leave it as a separate option. ================ 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) { ---------------- Naghasan wrote: > 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. > do you plan on raising kernel specific diagnostics even on the host side ? In > this case reusing this model will fully make sense. Not sure I understand the question. Why would we need raising kernel specific diagnostics on the host side? Is it against the SYCL design principal? 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
