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

User include files should not depend on library configuration (especially USE_GPU) #502

Open
ianmccul opened this issue Nov 9, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@ianmccul
Copy link
Contributor

ianmccul commented Nov 9, 2024

A small number of files that are installed as part of the public API include some conditional code hidden behind

#if defined(UNI_GPU)

This should not appear in header files in the user API in include/.

  1. Network.hpp
    This conditionally adds variables with type cutensornetNetworkDescriptor_t and cutensornetContractionOptimizerInfo_t to the Network_base class. This means it will produce an ABI incompatibility if UNI_GPU and UNI_CUQUANTUM in user code is different to how it was configured when the library was installed. Better option would be to put this behind an opaque type, say, NetworkImplementation (forward declared as an incomplete type in the header) and replace the cuda types with std::unique_ptr<NetworkImplementation>. (Note there are some minor technicalities with having a std::unique_ptr to an incomplete type, but it does work, and was designed to be allowed.)
    2. utils/cucomplex_arithmetic.hpp
    This is included from utils/utils.hpp. It declares a bunch of overloads for equality comparison and operator* between cuComplexand builtin types. I have no idea why it exists -- user code using cytnx normally wouldn't ever need to deal with cuFloatComplex and cuDoubleComplex directly.
  2. utils/complex_arithmetic.hpp
  3. Not sure why this header exists either - it declares a bunch of overloads for arithmetic operations between complex<double> / complex<float> and builtin types (double, float, int etc). Most of them already exist in namespace std anyway, but a few of them do not (which I guess is the purpose of the header...). Since they are declared in namespace cytnx, user code typically would not ever find them (unless using namespace cytnx, and then it is asking for trouble with two clashing overloads). Some of them are a bit strange, eg what does cytnx_complex64 operator/(const cytnx_bool &rn, const cytnx_complex64 &ln) do?! [edit: actually I think all of them exist in namespace std, via implicit conversion from builtin types to complex] But it also includes a copy-and-paste of the cuComplex operator== declarations that also appear in utils/cucomplex_arithmetic.hpp (but not the operator* overloads...)
  4. cytnx_error.hpp
    This looks like a lot of it was copy-pasted from https://github.com/NVIDIA/cuda-samples/blob/master/Common/helper_cuda.h . None of the CUDA-related functions are referred to anywhere else in the include files, so it looks like these could simply be removed from the public API. (also _cudaGetErrorEnum() could be replaced by the CUDA function cuGetErrorString(). But I suggest simply importing helper_cuda.h from the CUDA samples into the src/ directory and using it as-is.
@yingjerkao yingjerkao added the enhancement New feature or request label Nov 9, 2024
@yingjerkao
Copy link
Collaborator

We should clean up the code base...

@yingjerkao
Copy link
Collaborator

I also propose to put only public API header files in cytnx/include and keep the private headers next to the cpp files. In this way we have a clear separation of user level API and internal functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants