SciNim / nimblas

BLAS for Nim
http://scinim.github.io/nimblas
Apache License 2.0
42 stars 5 forks source link

wrap almost everything including complex matrix #5

Closed chimez closed 5 years ago

chimez commented 5 years ago
  1. Finish all work of wraping, and support complex matrix functions such as cblas_zgemm.
  2. For complex blas of Arraymancer #129 https://github.com/mratsim/Arraymancer/issues/129 @mratsim
andreaferretti commented 5 years ago

Thank you for your great work! I have only one issue: in cblas.h, the constants are defined like this:

enum CBLAS_ORDER {CblasRowMajor=101, CblasColMajor=102};
enum CBLAS_TRANSPOSE {CblasNoTrans=111, CblasTrans=112, CblasConjTrans=113};
enum CBLAS_UPLO {CblasUpper=121, CblasLower=122};
enum CBLAS_DIAG {CblasNonUnit=131, CblasUnit=132};
enum CBLAS_SIDE {CblasLeft=141, CblasRight=142};

and the size of an enum in C should be that of a C integer if I understand correctly (at least in popular implementations), while you changed sizeof(cint) to sizeof(int). Is there a reason for that?

chimez commented 5 years ago
  1. Because of "default integer type; bitwidth depends on architecture, but is always the same as a pointer" from "https://nim-lang.org/docs/system.html#int", I think it may be faster (and smaller on something like 8051) than cint.
  2. This is also the reason why parameters M, N, K, lda, ... use int instead of cint.
  3. cint is int32. I don't like int32.
  4. Changing all int to cint is also ok with me.
andreaferretti commented 5 years ago

Now that I think of it, I am confused. I had used int previously for these parameters, but now I ma wondering whether it worked by chance. By using that signature, I was passing to a compiled C library a data type of a different size from the one it expects, which can be unpredictable.

In the C world, we are passing (on most architecures) longs where ints are expected. I think the result of this is formally unspecified. I am not an expert, but as far as I know for architectures where 2s complement are used, it should do the right thing (also ur parameters are positive anyway). I am not sure whether depending on this is the right thing.

If you are sure this is not dangerous, I can go with all ints, otherwise maybe it would be better to use cints everywhere?

chimez commented 5 years ago
  1. OpenBLAS use its own int blasint ,see "https://github.com/xianyi/OpenBLAS/blob/develop/cblas.h#L56" and "https://github.com/xianyi/OpenBLAS/blob/ce3651516f12079f3ca2418aa85b9ad571c3a391/openblas_config_template.h#L40"
  2. MKL use its own int MKL_INT, see "https://software.intel.com/en-us/mkl-developer-reference-c-blas-code-examples"
  3. These ints can be int32 or int64, which depends on compile options.
  4. For nim, int on x86_32 is int32, on X86_64 is int64, so usually, no problem will be caused.
  5. For those who use 32bit-blas on X86_64 and 64bit-blas on X86_32, it might be a bug, see "https://github.com/numpy/numpy/pull/3916"
andreaferretti commented 5 years ago

Well, since these depend on compile options, I wonder what value they assume on common OS.

For openblas, the default seems to be int https://github.com/xianyi/OpenBLAS/blob/develop/common.h#L258-L266 but maybe the distro maintainers package it differently...

I am tempted to just go with int and only change if ever a problem arises.

chimez commented 5 years ago

For the type of parameters M,N,K,..., int type is fine. It passes the test of 2,000,000,000 X 2,000,000,000 complex matrix gemm, I think this is enough.

For the type of enum CBLAS_ORDER, ..., sizeof(enum) is not always equal to sizeof(int), see "https://stackoverflow.com/questions/1113855/is-the-sizeofenum-sizeofint-always". However, nim doesn't have a cenum and cast int64 to int32 is safe for a small number.

So, I think it's ok that all of integer use int instead of cint.

andreaferretti commented 5 years ago

Well then, I am accepting this PR, which uses int everywhere, and in case revisit this decision if a problem ever arises