[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 187751.
bader added a comment.

Applied comments from @ABataev.

- Split changes into two patches. This part contains front-end option enabling 
device specific macro. Changes adding driver option will be sent in a separate 
patch.
- Added LIT test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/Preprocessor/sycl-macro.cpp


Index: clang/test/Preprocessor/sycl-macro.cpp
===
--- /dev/null
+++ clang/test/Preprocessor/sycl-macro.cpp
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 %s -fsycl-is-device -E -dM | FileCheck %s
+
+// CHECK:#define __SYCL_DEVICE_ONLY__ 1
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1057,6 +1057,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) 
\
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2879,6 +2879,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX if specified in options
   Opts.OpenMPCUDAMode = Opts.OpenMPIsDevice && T.isNVPTX() &&
 Args.hasArg(options::OPT_fopenmp_cuda_mode);
Index: clang/include/clang/Driver/CC1Options.td
===
--- clang/include/clang/Driver/CC1Options.td
+++ clang/include/clang/Driver/CC1Options.td
@@ -836,8 +836,14 @@
 def fopenmp_host_ir_file_path : Separate<["-"], "fopenmp-host-ir-file-path">,
   HelpText<"Path to the IR file produced by the frontend for the host.">;
 
-} // let Flags = [CC1Option]
+//===--===//
+// SYCL Options
+//===--===//
 
+def fsycl_is_device : Flag<["-"], "fsycl-is-device">,
+  HelpText<"Generate code for SYCL device.">;
+
+} // let Flags = [CC1Option]
 
 
//===--===//
 // cc1as-only Options
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -217,6 +217,8 @@
 LANGOPT(CUDADeviceApproxTranscendentals, 1, 0, "using approximate 
transcendental functions")
 LANGOPT(GPURelocatableDeviceCode, 1, 0, "generate relocatable device code")
 
+LANGOPT(SYCLIsDevice  , 1, 0, "Generate code for SYCL device")
+
 LANGOPT(SizedDeallocation , 1, 0, "sized deallocation")
 LANGOPT(AlignedAllocation , 1, 0, "aligned allocation")
 LANGOPT(AlignedAllocationUnavailable, 1, 0, "aligned allocation functions are 
unavailable")


Index: clang/test/Preprocessor/sycl-macro.cpp
===
--- /dev/null
+++ clang/test/Preprocessor/sycl-macro.cpp
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 %s -fsycl-is-device -E -dM | FileCheck %s
+
+// CHECK:#define __SYCL_DEVICE_ONLY__ 1
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1057,6 +1057,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2879,6 +2879,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX 

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.
Herald added a subscriber: jdoerfert.

In D57768#1389012 , @rjmccall wrote:

> In D57768#1386862 , @Anastasia wrote:
>
> > - SYCL seem to require adding tight dependencies from the standard 
> > libraries into the compiler because many language features are hidden 
> > behind library classes. This is not common for Clang. We had a discussion 
> > about this issue during the implementation of OpenCL C++ and it was decided 
> > not to go this route for upstream Clang. Can you explain your current 
> > approach to implement this? I think @rjmccall  or @rsmith might need to be 
> > involved in this.
>
>
> I'd like to know more about this, but I'll point out that this isn't 
> unprecedented:
>
> - C compilers have hard-coded knowledge about `va_list`.
> - C++ compilers have hard-coded knowledge about `std::type_info` and 
> `std::initializer_list` (and possibly others I've forgotten).
>
>   Whether that's the right direction for SYCL, though, I can't say until I 
> understand more about what dependencies are being proposed.


In OpenCL we hard-coded `printf` btw! I think it would be good to understand 
how much of those is needed for SYCL. The fact that the entire language is 
implemented as a library is a bit worrying. But hopefully we can already map 
most of the things to some existing language constructs (address space 
qualifiers, function attributes, etc...).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D57768#1392975 , @bader wrote:

> In D57768#1386941 , @ABataev wrote:
>
> > In D57768#1386933 , @bader wrote:
> >
> > > In D57768#1386924 , @ABataev 
> > > wrote:
> > >
> > > > This definitely requires a test.
> > >
> > >
> > > @ABataev, I tried to find some tests on similar `-fcuda-is-device` and 
> > > `-fopenmp-is-device` options, but I wasn't able to find a dedicated test. 
> > > Could you suggest some examples testing similar functionality, please?
> >
> >
> > There are several similar tests:
> >  OpenMP/driver.c, Driver/openmp-offload.c, Driver/openmp-offload-gpu.c. 
> > There is no absolutely the same test for OpenMP, since OpenMP has mo 
> > similar req for the offloading.
>
>
> @ABataev thanks for the pointers. The uploaded patch adds two options:
>
> - fsycl-is-device (front-end option)
> - sycl-device-only (driver option)
>
>   The driver tests you mention validate driver logic enabled by new options, 
> which is not part of this test and I was going to add it later. I can split 
> the patch and remove new driver option and leave only front-end option. 
> Another option is to add driver logic that invokes the front-end compiler in 
> "device only" mode. Which option do you prefer?


Yes, split the patch into 2. But you still need to add the tests for each of 
them.
You need to add at least 2 tests:

1. The test that checks that the driver option is converted into the frontend 
option (driver with `--sycl-device-only` calls the frontend with 
`-fsycl-is-device`.
2. The test that checks that the frontend option `-fsycl-is-device` adds a new 
macro definition `__SYCL_DEVICE_ONLY__ 1`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D57768#1386941 , @ABataev wrote:

> In D57768#1386933 , @bader wrote:
>
> > In D57768#1386924 , @ABataev wrote:
> >
> > > This definitely requires a test.
> >
> >
> > @ABataev, I tried to find some tests on similar `-fcuda-is-device` and 
> > `-fopenmp-is-device` options, but I wasn't able to find a dedicated test. 
> > Could you suggest some examples testing similar functionality, please?
>
>
> There are several similar tests:
>  OpenMP/driver.c, Driver/openmp-offload.c, Driver/openmp-offload-gpu.c. There 
> is no absolutely the same test for OpenMP, since OpenMP has mo similar req 
> for the offloading.


@ABataev thanks for the pointers. The uploaded patch adds two options:

- fsycl-is-device (front-end option)
- sycl-device-only (driver option)

The driver tests you mention validate driver logic enabled by new options, 
which is not part of this test and I was going to add it later.
I can split the patch and remove new driver option and leave only front-end 
option.
Another option is to add driver logic that invokes the front-end compiler in 
"device only" mode.
Which option do you prefer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D57768#1386862 , @Anastasia wrote:

> - SYCL seem to require adding tight dependencies from the standard libraries 
> into the compiler because many language features are hidden behind library 
> classes. This is not common for Clang. We had a discussion about this issue 
> during the implementation of OpenCL C++ and it was decided not to go this 
> route for upstream Clang. Can you explain your current approach to implement 
> this? I think @rjmccall  or @rsmith might need to be involved in this.


I'd like to know more about this, but I'll point out that this isn't 
unprecedented:

- C compilers have hard-coded knowledge about `va_list`.
- C++ compilers have hard-coded knowledge about `std::type_info` and 
`std::initializer_list` (and possibly others I've forgotten).

Whether that's the right direction for SYCL, though, I can't say until I 
understand more about what dependencies are being proposed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D57768#1386933 , @bader wrote:

> In D57768#1386924 , @ABataev wrote:
>
> > This definitely requires a test.
>
>
> @ABataev, I tried to find some tests on similar `-fcuda-is-device` and 
> `-fopenmp-is-device` options, but I wasn't able to find a dedicated test. 
> Could you suggest some examples testing similar functionality, please?


There are several similar tests:
OpenMP/driver.c, Driver/openmp-offload.c, Driver/openmp-offload-gpu.c. There is 
no absolutely the same test for OpenMP, since OpenMP has mo similar req for the 
offloading.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D57768#1386924 , @ABataev wrote:

> This definitely requires a test.


@ABataev, I tried to find some tests on similar `-fcuda-is-device` and 
`-fopenmp-is-device` options, but I wasn't able to find a dedicated test. Could 
you suggest some examples testing similar functionality, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev requested changes to this revision.
ABataev added a comment.
This revision now requires changes to proceed.

This definitely requires a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added subscribers: rjmccall, rsmith.
Anastasia added a comment.

In D57768#1386791 , @bader wrote:

> In D57768#1386754 , @Naghasan wrote:
>
> > LGTM
> >
> > Side note: might be good to also involve @Anastasia, as some of the future 
> > patches will overlap with OpenCL.
>
>
> Sure. I'll add @Anastasia as a reviewer to the relevant patches.
>
> Thanks,
> Alexey


Hi,

Sorry for the delay. I was planning to reply on the RFC this week after 
finishing looking at your prototype in github. Seems quite a substantial amount 
of work!

There are a number of big architectural aspects of Clang that this work 
affects. My personal feeling is that some more senior developers in Clang 
architecture should provide feedback before we go ahead with detailed reviews.

Particularly, the following needs clarification:

- SYCL seem to require adding tight dependencies from the standard libraries 
into the compiler because many language features are hidden behind library 
classes. This is not common for Clang. We had a discussion about this issue 
during the implementation of OpenCL C++ and it was decided not to go this route 
for upstream Clang. Can you explain your current approach to implement this? I 
think @rjmccall  or @rsmith might need to be involved in this.

- I am not sure how the change of direction for OpenCL C++ to just enabling C++ 
in OpenCL would affect your work now? Particularly we are establishing a lot of 
rules in the areas of interplay between OpenCL features and  C++.  Address 
space handling is one example here. As far as I am aware SYCL doesn't detail 
many of these rules either. So I am wondering how it would work... would you 
just inherit the same rules? Also keep in mind they are not documented anywhere 
yet other than the source code.

- What is your solution for integration with SPIR-V and how does it relate to 
our previous discussions in October: 
http://lists.llvm.org/pipermail/cfe-dev/2018-October/059974.html

- Can you explain the purpose of 
https://github.com/intel/llvm/tree/sycl/clang/lib/CodeGen/OclCxxRewrite that 
you are adding to Clang?

I will follow up on RFC as well so we can have a wider discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D57768#1386754 , @Naghasan wrote:

> LGTM
>
> Side note: might be good to also involve @Anastasia, as some of the future 
> patches will overlap with OpenCL.


Sure. I'll add @Anastasia as a reviewer to the relevant patches.

Thanks,
Alexey


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Victor Lomuller via Phabricator via cfe-commits]

Naghasan accepted this revision.
Naghasan added a subscriber: Anastasia.
Naghasan added a comment.

LGTM

Side note: might be good to also involve @Anastasia, as some of the future 
patches will overlap with OpenCL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Ronan Keryell via Phabricator via cfe-commits]

keryell accepted this revision.
keryell added a comment.
This revision is now accepted and ready to land.

That looks good. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.
bader added reviewers: keryell, Naghasan.
Herald added subscribers: cfe-commits, ebevhan.
Herald added a project: clang.

-fsycl-is-device enables compilation of the device part of SYCL source
file.

Patch by Mariya Podchishchaeva 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57768

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/InitPreprocessor.cpp


Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1056,6 +1056,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2875,6 +2875,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX if specified in options
   Opts.OpenMPCUDAMode = Opts.OpenMPIsDevice && T.isNVPTX() &&
 Args.hasArg(options::OPT_fopenmp_cuda_mode);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3150,6 +3150,9 @@
 defm underscoring : BooleanFFlag<"underscoring">, Group;
 defm whole_file : BooleanFFlag<"whole-file">, Group;
 
+// SYCL options
+def sycl_device_only : Flag<["--"], "sycl-device-only">,
+  HelpText<"Compile SYCL code for device only">;
 
 include "CC1Options.td"
 
Index: clang/include/clang/Driver/CC1Options.td
===
--- clang/include/clang/Driver/CC1Options.td
+++ clang/include/clang/Driver/CC1Options.td
@@ -836,8 +836,14 @@
 def fopenmp_host_ir_file_path : Separate<["-"], "fopenmp-host-ir-file-path">,
   HelpText<"Path to the IR file produced by the frontend for the host.">;
 
-} // let Flags = [CC1Option]
+//===--===//
+// SYCL Options
+//===--===//
 
+def fsycl_is_device : Flag<["-"], "fsycl-is-device">,
+  HelpText<"Generate code for SYCL device.">;
+
+} // let Flags = [CC1Option]
 
 
//===--===//
 // cc1as-only Options
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -215,6 +215,8 @@
 LANGOPT(CUDADeviceApproxTranscendentals, 1, 0, "using approximate 
transcendental functions")
 LANGOPT(GPURelocatableDeviceCode, 1, 0, "generate relocatable device code")
 
+LANGOPT(SYCLIsDevice  , 1, 0, "Generate code for SYCL device")
+
 LANGOPT(SizedDeallocation , 1, 0, "sized deallocation")
 LANGOPT(AlignedAllocation , 1, 0, "aligned allocation")
 LANGOPT(AlignedAllocationUnavailable, 1, 0, "aligned allocation functions are 
unavailable")


Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1056,6 +1056,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2875,6 +2875,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX if specified in options
   Opts.OpenMPCUDAMode = Opts.OpenMPIsDevice && T.isNVPTX() &&
 Args.hasArg(options::OPT_fopenmp_cuda_mode);
Index: clang/include/clang/Driver/Options.td
===
---