oneapi-src / oneMKL

oneAPI Math Kernel Library (oneMKL) Interfaces
Apache License 2.0
607 stars 155 forks source link

Improper namespace for BLAS domain #59

Closed vrpascuzzi closed 3 years ago

vrpascuzzi commented 3 years ago

Summary

Namespace scopes in oneAPI are of the form oneapi::mkl::domain, e.g. oneapi::mkl::blas and oneapi::mkl::rng. In oneMKL, the RNG domain libraries are of the form oneapi::mkl::rng::library, e.g. oneapi::mkl::rng::mklgpu.

Problem statement

The namespace convention in oneMKL differs between the BLAS and RNG domain libraries; where BLAS uses oneapi::mkl::library for library namespaces, RNG uses -- more appropriately -- oneapi::mkl::rng::library. This observation was made when reading through Integrating a Third-party Library to oneAPI Math Kernel Library (oneMKL) Interfaces:

python scripts/generate_backend_api.py include/oneapi/mkl/blas.hpp \                                  # Base header file
                                       include/oneapi/mkl/blas/detail/newlib/onemkl_blas_newlib.hpp \ # Output header file
                                       oneapi::mkl::newlib                                            # Wrappers namespace

where I believe it should be:

python scripts/generate_backend_api.py include/oneapi/mkl/blas.hpp \                                  # Base header file
                                       include/oneapi/mkl/blas/detail/newlib/onemkl_blas_newlib.hpp \ # Output header file
                                       oneapi::mkl::domain::newlib                                    # Wrappers namespace

With the former instruction, adding a new third-party library to the RNG domain generates an incorrect namespace, e.g. onemkl_rng_curand.hpp:

...
namespace oneapi {
namespace mkl {
namespace curand {} // namespace curand
} // namespace mkl
} // namespace oneapi

while it should be:

...
namespace oneapi {
namespace mkl {
namespace rng {
namespace curand {} // namespace curand
} // namespace rng
} // namespace mkl
} // namespace oneapi

Preferred solution

The BLAS domain should be namespace'd consistently with the RNG domain. That is, more suitable would be:

oneapi::mkl::mklcpu --> oneapi::mkl::blas::mklcpu
oneapi::mkl::mklgpu --> oneapi::mkl::blas::mklgpu
oneapi::mkl::netlib --> oneapi::mkl::blas::netlib
oneapi::mkl::cublas --> oneapi::mkl::blas::cublas

which matches the RNG domain:

oneapi::mkl::rng::mklcpu
oneapi::mkl::rng::mklgpu

This also better reflects the header file naming convention, e.g. onemkl_blas_netlib.hpp.

If this is an acceptable change, I will make a new PR.

vmalia commented 3 years ago

Thank you @vrpascuzzi ! We indeed missed "blas" namespace there. If you have a patch, feel free to open a PR. If not, we will fix it soon.

vrpascuzzi commented 3 years ago

Thanks for following up, @vmalia. I will get to it this week.