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

[Ruyman via Phabricator via cfe-commits]

Ruyk 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")
----------------
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.



================
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">;
 
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3989
+    // Ensure the default version in SYCL mode is 1.2.1
+    CmdArgs.push_back("-sycl-std=1.2.1");
+  }
----------------
This should probably change to 2015 if we are allowing that, just for consistency


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72857