[Driver][SYCL] Provide ability to use an external host compiler #3530

mdtoguchi · 2021-04-10T18:39:58Z

The default toolchain when performing -fsycl compilations is to use the
clang compiler for the device and host compilations. Enable the ability
to use an external host compiler (e.g. gcc/g++) to be used when creating
the host objects. This introduces 2 options:

-fsycl-host-compiler=
When specified, this option informs the compiler driver that the host
compilation step that is performed as part of the greater compilation
flow will be performed by the compiler . It is expected that
is the compiler to be called, either by name (in which the PATH
will be used to discover it) or a fully qualified directory with
compiler to invoke. This option is only useful when used with when
'-fsycl' is provided on the command line.

-fsycl-host-compiler-options="opts"
Passes along the space separated quoted "opts" string as option
arguments to the compiler specified with the -fsycl-host-compiler=
option. It is expected that the options used here are compatible with
the compiler specified via -fsycl-host-compiler=. This option
does not have any affect without -fsycl-host-compiler=.

The default toolchain when performing -fsycl compilations is to use the clang compiler for the device and host compilations. Enable the ability to use an external host compiler (e.g. gcc/g++) to be used when creating the host objects. This introduces 2 options: -fsycl-host-compiler=<arg> When specified, this option informs the compiler driver that the host compilation step that is performed as part of the greater compilation flow will be performed by the compiler <arg>. It is expected that <arg> is the compiler to be called, either by name (in which the PATH will be used to discover it) or a fully qualified directory with compiler to invoke. This option is only useful when used with when '-fsycl' is provided on the command line. -fsycl-host-compiler-options="opts" Passes along the space separated quoted "opts" string as option arguments to the compiler specified with the -fsycl-host-compiler=<arg> option. It is expected that the options used here are compatible with the compiler specified via -fsycl-host-compiler=<arg>. This option does not have any affect without -fsycl-host-compiler=<arg>.

clang/include/clang/Driver/Options.td

clang/lib/Driver/ToolChains/Clang.cpp

pvchupin

doc ok

AaronBallman

Mostly just small nits from me.

clang/lib/Driver/ToolChains/Clang.cpp

AaronBallman

LGTM!

kbobrovs

LGTM. Thanks!

mdtoguchi · 2021-04-13T21:36:00Z

@AGindinson , can you take a look?

AGindinson

Is it possible to add E2E tests of some sort into llvm-test-suite or sycl tests?

clang/include/clang/Driver/Options.td

clang/lib/Driver/ToolChains/Clang.cpp

clang/test/Driver/sycl-host-compiler.cpp

clang/lib/Driver/ToolChains/Clang.cpp

mdtoguchi · 2021-04-15T00:28:04Z

While working on an E2E test, I found issues at runtime. When using the external compiler and using -I

/include/sycl there would be a runtime fault. Poking around I found that if I used the clang++ as an external host compiler and tried it would still fail in the same manner. Forcing the usage of -internal-isystem <dir>/include/sycl allows for the runtime to succeed. I've reached out to @premanandrao for help, but maybe @AaronBallman has any ideas what is going on?

AGindinson

The changes LGTM on the whole. I'm joining to the help request for @premanandrao or @AaronBallman on the matter of E2E testing.

AaronBallman · 2021-04-16T10:59:43Z

While working on an E2E test, I found issues at runtime. When using the external compiler and using -I

/include/sycl there would be a runtime fault. Poking around I found that if I used the clang++ as an external host compiler and tried it would still fail in the same manner. Forcing the usage of -internal-isystem <dir>/include/sycl allows for the runtime to succeed. I've reached out to @premanandrao for help, but maybe @AaronBallman has any ideas what is going on?

I'm not clear on how we position /include/sycl (whether we believe it's a system header because it's part of the implementation of the compiler or whether we treat it like any other third-party library like openssl, etc), but I suspect we treat it as a system header. Based on that, I think you want to use -cxx-isystem rather than -I so that it is very clear that 1) it's a system header and all the usual system header hacks apply to it, 2) it's a C++-only system header, and 3) it's only available through angle bracket inclusion and not quoted inclusion. That said, this is a bit out of my wheelhouse and I may be repeating information you've already considered.

mdtoguchi · 2021-04-16T17:12:44Z

While working on an E2E test, I found issues at runtime. When using the external compiler and using -I
/include/sycl there would be a runtime fault. Poking around I found that if I used the clang++ as an external host compiler and tried it would still fail in the same manner. Forcing the usage of -internal-isystem <dir>/include/sycl allows for the runtime to succeed. I've reached out to @premanandrao for help, but maybe @AaronBallman has any ideas what is going on?

I'm not clear on how we position /include/sycl (whether we believe it's a system header because it's part of the implementation of the compiler or whether we treat it like any other third-party library like openssl, etc), but I suspect we treat it as a system header. Based on that, I think you want to use -cxx-isystem rather than -I so that it is very clear that 1) it's a system header and all the usual system header hacks apply to it, 2) it's a C++-only system header, and 3) it's only available through angle bracket inclusion and not quoted inclusion. That said, this is a bit out of my wheelhouse and I may be repeating information you've already considered.

I've tried various things including -cxx-isystem. Unfortunately, only using -internal-isystem allowed for this to work, which doesn't seem to be very friendly when dealing with an external compiler invocation.

AaronBallman · 2021-04-16T17:26:11Z

I've tried various things including -cxx-isystem. Unfortunately, only using -internal-isystem allowed for this to work, which doesn't seem to be very friendly when dealing with an external compiler invocation.

That seems pretty strange to me and may be worth opening an issue to try to track down the root cause of. It seems odd that we need to use the undocumented flag (https://clang.llvm.org/docs/ClangCommandLineReference.html#include-path-management) to get things to work.

mdtoguchi · 2021-04-16T19:17:54Z

Looks like my environment is somewhat tainted. I had a CPLUS_INCLUDE_PATH setting to an internal (archived) set of the headers. The combination of this and the -internal-isystem allowed things to work but not clear if it is just happenstance. I went ahead and uploaded my small E2E test to see what happens with a clean environment with our testing.

premanandrao · 2021-04-16T20:03:27Z

Looks like my environment is somewhat tainted. I had a CPLUS_INCLUDE_PATH setting to an internal (archived) set of the headers. The combination of this and the -internal-isystem allowed things to work but not clear if it is just happenstance. I went ahead and uploaded my small E2E test to see what happens with a clean environment with our testing.

This may explain why I am not able to reproduce this failure. :-)

sycl/test/regression/fsycl-host-compiler.cpp

AGindinson

LGTM. A similar E2E Windows test could be "intuitively nice", but I personally believe the Linux one provides sufficient coverage for the Driver part.

dm-vodopyanov

Thanks, sycl/test/regression part LGTM

dm-vodopyanov · 2021-04-19T10:19:30Z

A similar E2E Windows test could be "intuitively nice

Missed this. Agree, E2E test on Windows with msvc will be nice to have test, but only Linux one looks ok.

dm-vodopyanov

sycl/test/regression part LGTM

…Windows

mdtoguchi · 2021-04-19T19:41:09Z

Everything should be accounted for, could I get a final look? @kbobrovs, @pvchupin , @AaronBallman, @dm-vodopyanov

pvchupin

doc ok

dm-vodopyanov

sycl/test/regression part LGTM

premanandrao

FE changes continue to look good to me.

mdtoguchi requested review from AaronBallman, AGindinson, elizabethandrews, kbobrovs, premanandrao and pvchupin as code owners April 10, 2021 18:39

Clang format

972a6ed

kbobrovs reviewed Apr 11, 2021

View reviewed changes

pvchupin previously approved these changes Apr 11, 2021

View reviewed changes

Address review comments

f38a286

mdtoguchi dismissed pvchupin’s stale review via f38a286 April 11, 2021 18:12

AaronBallman reviewed Apr 12, 2021

View reviewed changes

mdtoguchi added 2 commits April 12, 2021 17:37

Address additional review comments

aefc45a

Adjust how to determine using MSVC type compiler

571e8df

AaronBallman previously approved these changes Apr 13, 2021

View reviewed changes

kbobrovs previously approved these changes Apr 13, 2021

View reviewed changes

AGindinson reviewed Apr 14, 2021

View reviewed changes

Address review comments from Artem

bc068dd

mdtoguchi dismissed stale reviews from kbobrovs and AaronBallman via bc068dd April 15, 2021 00:23

mdtoguchi added 2 commits April 14, 2021 19:14

Add additional option test for cl type host compilers

2ff9dfd

Improve error handling for missing compiler arg

7d81f75

AGindinson requested a review from AaronBallman April 16, 2021 08:56

AGindinson reviewed Apr 16, 2021

View reviewed changes

Add E2E test

a68debe

mdtoguchi requested a review from a team as a code owner April 16, 2021 19:13

mdtoguchi requested a review from dm-vodopyanov April 16, 2021 19:13

dm-vodopyanov reviewed Apr 16, 2021

View reviewed changes

sycl/test/regression/fsycl-host-compiler.cpp Outdated Show resolved Hide resolved

Update test to use -fsycl-host-compiler-options

b998d30

AGindinson previously approved these changes Apr 19, 2021

View reviewed changes

AGindinson requested review from kbobrovs, dm-vodopyanov and pvchupin April 19, 2021 09:36

dm-vodopyanov previously approved these changes Apr 19, 2021

View reviewed changes

Add E2E for Windows

2d66de2

mdtoguchi dismissed stale reviews from dm-vodopyanov and AGindinson via 2d66de2 April 19, 2021 15:24

AGindinson previously approved these changes Apr 19, 2021

View reviewed changes

dm-vodopyanov previously approved these changes Apr 19, 2021

View reviewed changes

Add sycl default library to link when using -fsycl-host-compiler for …

b39f7ab

…Windows

mdtoguchi dismissed stale reviews from dm-vodopyanov and AGindinson via b39f7ab April 19, 2021 17:28

AGindinson approved these changes Apr 19, 2021

View reviewed changes

pvchupin approved these changes Apr 19, 2021

View reviewed changes

dm-vodopyanov approved these changes Apr 19, 2021

View reviewed changes

premanandrao approved these changes Apr 20, 2021

View reviewed changes

bader merged commit 6f0ad1a into intel:sycl Apr 20, 2021

[Driver][SYCL] Provide ability to use an external host compiler #3530

[Driver][SYCL] Provide ability to use an external host compiler #3530

Uh oh!

Conversation

mdtoguchi commented Apr 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pvchupin left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

mdtoguchi commented Apr 13, 2021

Uh oh!

AGindinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdtoguchi commented Apr 15, 2021

Uh oh!

AGindinson left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Apr 16, 2021

Uh oh!

mdtoguchi commented Apr 16, 2021

Uh oh!

AaronBallman commented Apr 16, 2021

Uh oh!

mdtoguchi commented Apr 16, 2021

Uh oh!

premanandrao commented Apr 16, 2021

Uh oh!

Uh oh!

AGindinson left a comment

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov left a comment

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov commented Apr 19, 2021

Uh oh!

dm-vodopyanov left a comment

Choose a reason for hiding this comment

Uh oh!

mdtoguchi commented Apr 19, 2021

Uh oh!

pvchupin left a comment

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov left a comment

Choose a reason for hiding this comment