Patch by Mariya Podchishchaeva <firstname.lastname@example.org>
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.
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.
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.
@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:
- 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.
- The test that checks that the frontend option -fsycl-is-device adds a new macro definition __SYCL_DEVICE_ONLY__ 1
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...).