Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do no pollute the top level namespace #3374

Open
keryell opened this issue Mar 17, 2021 · 9 comments
Open

Do no pollute the top level namespace #3374

keryell opened this issue Mar 17, 2021 · 9 comments
Assignees
Labels
bug Something isn't working confirmed

Comments

@keryell
Copy link
Contributor

keryell commented Mar 17, 2021

While playing with FPGA libraries we got some compiling conflicts with some ::ap_int type.

It happens that while https://github.com/intel/llvm/blob/sycl/sycl/include/CL/__spirv/spirv_types.hpp declares things inside ::__spv there are also some random declarations at the end of the file, like RPipeTy, WPipeTy, ConstantPipeStorage, ap_int.

If these declarations are useful, they should go into some detail or extension namespaces.

There are some other top-level declarations in __spirv but they are starting with __ which is OK since it is a kind of private implementation world in C/C++ provided there is some collaborative damage control.

@keryell keryell added the bug Something isn't working label Mar 17, 2021
@MrSidims MrSidims self-assigned this Mar 18, 2021
@MrSidims
Copy link
Contributor

That is bad, not sure why I named ConstantPipeStorage like that and not like __spirv_ConstantPipeStorage. And it might be tricky to change it now (may be in staggered way).
@tiwaria1 @GarveyJoe can users operate with ap_int type directly or they supposed to use wrappers around it? Aka can we put it in a namespace safely (for sure again in a staggered way)? And a following question to Joe: previously user API for pipes was placed in sycl:: namespace, later it was moved to sycl::INTEL namespace (keeping legacy one for a while). Can we now safely remove legacy version completely?

@tiwaria1
Copy link
Contributor

Users are supposed to use the wrapper ac_* types (ac_int, ac_fixed etc). We can move ap_int into a namespace. This will impact the ac_* type headers and also: the spirv_ops.hpp header. So these changes must occur together.

@keryell
Copy link
Contributor Author

keryell commented Mar 18, 2021

It looks like it is only used from https://github.com/intel/llvm/blob/sycl/sycl/include/CL/__spirv/spirv_ops.hpp so it should be simple to fix.

@GarveyJoe
Copy link
Contributor

@MrSidims it seems at least one of our code samples is still using the sycl:: namespace version. Once that is updated I think it's safe to deprecate the old namespace.

@keryell
Copy link
Contributor Author

keryell commented Mar 18, 2021

Just to clarify: we are talking about :: here, not ::sycl or whatever. I guess your new and old libraries could be just updated to use the functions in ::some_detail instead?

@MrSidims
Copy link
Contributor

MrSidims commented Mar 19, 2021

It's about sycl::pipes vs sycl::INTEL::pipes in tests and user tutorials, like: https://github.com/oneapi-src/oneAPI-samples/blob/master/DirectProgramming/DPC%2B%2BFPGA/Tutorials/Features/pipes/src/pipes.cpp , so it's not about backwards compatibility, more like about user friendliness.

@keryell
Copy link
Contributor Author

keryell commented Oct 12, 2021

We need to make progress on this and it has been blocking triSYCL/sycl#125 by a side effect for too long.
@tiwaria1 "So these changes must occur together." looks like a deadlock that will never happen. ;-)
I am thinking to introduce on your side triSYCL/sycl#125 (comment), but as an inline namespace first or adding a using namespace where the polluting ac_int needs to appear, so the old code can still work, but you can test the new code too.
Once you have modernized all the tutorials/users code/... you can remove the inline namespace or the namespace alias.
It does not solve our problem directly because there will still be some conflicts and so we will have our own branch in the meantime but it put you on an improving track and at some point we will converge.

@MrSidims
Copy link
Contributor

MrSidims commented Oct 12, 2021

@keryell sure, I'll return to #3986 tomorrow. We now have updated FPGA emulator, with which PipeStorage name change shouldn't cause any issues. Not sure if I'll be able to rename ap/ac/hls_int/float so easily, but I'll take a look as well.

@MrSidims
Copy link
Contributor

MrSidims commented Oct 13, 2021

Updated the PR. Regarding ap_int type, I'll sync with @tiwaria1 today to ensure that we update both the wrapper headers and the SYCL headers simultaneously.

Old namespace for pipes will be removed shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working confirmed
Projects
None yet
Development

No branches or pull requests

5 participants