Skip to content

Restructure code into runtime, sycl, compiler subdirs and corresponding namespaces, implement Xilinx proposal for multiple entrypoints#216

Merged
illuhad merged 4 commits intomasterfrom
restructure-source-code
Mar 11, 2020
Merged

Restructure code into runtime, sycl, compiler subdirs and corresponding namespaces, implement Xilinx proposal for multiple entrypoints#216
illuhad merged 4 commits intomasterfrom
restructure-source-code

Conversation

@illuhad
Copy link
Collaborator

@illuhad illuhad commented Mar 6, 2020

This PR is a major restructuring of the hipSYCL code base. The motivation is that, with the new runtime, we will have a very strict separation between code from the SYCL interface headers and the SYCL runtime. I would also like to underline this via namespaces and the directory structure. Therefore, this PR moves to the following code structure:

Component Header files Source files Namespace
SYCL Interface include/hipSYCL/sycl src/sycl hipsycl::sycl
SYCL Runtime include/hipSYCL/runtime src/runtime hipsycl::rt
Compiler (clang plugin) include/hipSYCL/compiler src/compiler hipsycl::compiler

This will also give me a good structure to develop the new runtime against.

Note that I have not separated all the existing runtime code from the SYCL interface in this PR; this will be done when the new runtime comes where I will need to remove the old runtime bits anyway.

Because the SYCL headers are now in the generic namespace hipsycl::sycl, this has allowed me to implement the Xilinx proposal to generalize namespaces and allow a SYCL program to use multiple SYCL implementations simultaneously (see here: triSYCL/triSYCL#249).
In short:

  • #include <CL/sycl.hpp> exposes the SYCL api in the namespace cl::sycl::
  • #include <SYCL/sycl.hpp> exposes the SYCL api in the namespace sycl::
  • #include <hipSYCL/sycl.hpp> exposes the SYCL api in the namespace hipsycl::.
    Note that this feature is mostly a side effect of the larger reshuffling of code, I have not yet tested it fully :-)

This is still marked as WIP because I would like to look at this some more to make sure it's good to go.

…ng namespaces, implement Xilinx proposal for multiple entrypoints
@illuhad illuhad changed the title Restructure code into runtime, sycl, compiler subdirs and correspondig ng namespaces, implement Xilinx proposal for multiple entrypoints Restructure code into runtime, sycl, compiler subdirs and corresponding namespaces, implement Xilinx proposal for multiple entrypoints Mar 6, 2020
@keryell
Copy link
Contributor

keryell commented Mar 7, 2020

That sounds like a very good idea! ;-)
Soon we will be able to have 5 different SYCL implementations running at the same time and handling various accelerators! :-)

@illuhad
Copy link
Collaborator Author

illuhad commented Mar 9, 2020

@keryell 5 Implementations? Is sycl-gtx also going to implement this (I had the impression the project was mostly dormant), or is there a SYCL implementation that I am not aware of?

@illuhad illuhad marked this pull request as ready for review March 9, 2020 13:59
@keryell
Copy link
Contributor

keryell commented Mar 9, 2020

@keryell 5 Implementations? Is sycl-gtx also going to implement this (I had the impression the project was mostly dormant), or is there a SYCL implementation that I am not aware of?

OK, let's say that 5 was an optimistic upper bound estimate today. :-)

@illuhad illuhad merged commit 3e5f698 into master Mar 11, 2020
@illuhad illuhad deleted the restructure-source-code branch March 11, 2020 16:48
@psalz
Copy link
Member

psalz commented Mar 19, 2020

A possibly unintended consequence of this change is that it's no longer possible to forward declare SYCL types within the cl::sycl namespace, if they are actually defined within hipsycl::sycl.

See: https://www.compiler-explorer.com/z/L4hMzY

Edit - clarified the example code a bit more. Imagine the forward declaration at the top to be in a header of a hipSYCL user, and the following two namespace blocks being the way hipSYCL currently exports cl::sycl::queue from its header.

@illuhad
Copy link
Collaborator Author

illuhad commented Mar 19, 2020

What would be the use case for this? Why can't the user just #include <CL/sycl.hpp>?

triSYCL implements this exactly the same way, and will also be affected by this. I don't see a lot options around this, if SYCL should become more general than OpenCL as per the generalization proposal (making the cl:: namespace deprecated) while still retaining some degree of backwards compatibility.

@psalz
Copy link
Member

psalz commented Mar 19, 2020

I've been forward declaring SYCL types in Celerity headers whenever I don't need the actual implementation, for example when I'm only using pointers to SYCL types -- to reduce compile times.

@keryell
Copy link
Contributor

keryell commented Mar 19, 2020

@psalz for the same reason I think you could not forward declare std::string...

Perhaps this is something to add to SYCL spec... Do not do this. :-)

You are playing here with some implementation details...

@psalz
Copy link
Member

psalz commented Mar 20, 2020

I actually did some googling on whether forward declaring library types was considered bad practice, and it doesn't seem like there is any broad consensus on it. However apparently the C++ standard explicitly forbids it, so maybe it really is something worth considering adding to the spec.

It's not a big deal, just something I noticed when trying to compile Celerity against the newest version of hipSYCL.

@keryell
Copy link
Contributor

keryell commented Mar 20, 2020

@psalz since you seem at speed on this subject, you can send a PR on the SYCL specification. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants