Page MenuHomePhabricator

[SYCL] Add clang front-end option to enable SYCL device compilation flow.
ClosedPublic

Authored by bader on Feb 5 2019, 10:28 AM.

Details

Summary

Patch by Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Event Timeline

bader created this revision.Feb 5 2019, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 10:28 AM
keryell accepted this revision.Feb 5 2019, 11:03 AM

That looks good. Thanks.

This revision is now accepted and ready to land.Feb 5 2019, 11:03 AM
Naghasan accepted this revision.Feb 6 2019, 1:22 AM
Naghasan added a subscriber: Anastasia.

LGTM

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

bader added a comment.Feb 6 2019, 2:29 AM

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

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.

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

ABataev requested changes to this revision.Feb 6 2019, 5:22 AM
ABataev added a subscriber: ABataev.

This definitely requires a test.

This revision now requires changes to proceed.Feb 6 2019, 5:22 AM
bader added a comment.Feb 6 2019, 5:28 AM

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?

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.

  • 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.

bader added a comment.Feb 11 2019, 7:08 AM

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?

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
  • 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...).

bader updated this revision to Diff 187751.Feb 21 2019, 2:58 AM

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
bader retitled this revision from [SYCL] Add SYCL device compilation flow. to [SYCL] Add clang front-end option to enable SYCL device compilation flow..Feb 21 2019, 2:59 AM
bader edited the summary of this revision. (Show Details)
ABataev added inline comments.Feb 21 2019, 6:13 AM
clang/test/Preprocessor/sycl-macro.cpp
2

Add a test that without this option __SYCL_DEVICE_ONLY__ is not defined

bader updated this revision to Diff 187785.Feb 21 2019, 6:42 AM

Add check that SYCL specific macro is not enabled by default.

This revision is now accepted and ready to land.Feb 21 2019, 6:46 AM