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

(double*) instead of (void*) for complex numbers in CBLAS #1322

Closed
claudius-hubig opened this issue Oct 6, 2017 · 14 comments

Comments

Projects
None yet
4 participants
@claudius-hubig
Copy link

commented Oct 6, 2017

(copy of Debian bug 877883 )

Dear Maintainer,

OpenBLAS in version 0.2.19-3 currently installed on Debian Stretch as well as the version 0.2.20+ds-4 in Testing declares the cblas_*() as taking double* instead of void* for complex values. As an example, it declares cblas_zdscal as

void cblas_zdscal(OPENBLAS_CONST blasint N, OPENBLAS_CONST double alpha, double *X, OPENBLAS_CONST blasint incX);

This function takes an array of complex numbers X and scales them by the real value alpha.

In contrast to this, all other packages in Debian as well as the Intel MKL define cblas_zdscal as taking a void* for X, i.e.:

void cblas_zdscal(const int N, const double alpha, void *X, const int incX);

See e.g. Debian Codesearch or the MKL reference or the Netlib cblas.h.

The same difference applies to the cblas_zscal() function and all others:

$ dpkg -S /usr/include/cblas.h; dpkg -S /usr/include/openblas/cblas.h; grep zscal /usr/include/openblas/cblas.h /usr/include/cblas.h
libblas-dev: /usr/include/cblas.h
libopenblas-dev: /usr/include/openblas/cblas.h
/usr/include/openblas/cblas.h:void cblas_zscal(OPENBLAS_CONST blasint N, OPENBLAS_CONST double *alpha, double *X, OPENBLAS_CONST blasint incX);
/usr/include/cblas.h:void cblas_zscal(const int N, const void *alpha, void *X, const int incX);

I have only noticed the issue now as cblas.h has become part of the alternatives system and is currently directing to the OpenBLAS version in testing. In Stretch, if also libblas-dev is installed, its cblas.h is used and the difference hence hidden away.

I suspect that this problem is not more widespread because people tend to cast their arguments to (void*) explicitly, but the conversion std::complex<double>* => void* => double* is probably actually undefined…

There was also some discussion in 2013, but it seemed to go nowhere.

I would be very thankful if there was some way to avoid the explicit casts to (void*) in calls specifically for OpenBLAS.

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Oct 6, 2017

These are probably "historic" differences, already present in K.Goto's GotoBLAS from about 10 years ago.

@svillemot

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

ATLAS and GSL CBLAS also uses void * for complex array.

It is clearly a problem if OpenBLAS does not provide the same API as the other CBLAS implementations.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

Netlib has this:
void cblas_zdscal(const int N, const double alpha, void *X, const int incX);
Everybody else is wrong and deviant from reference implementation?

@svillemot

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

@brada4 No, everybody except OpenBLAS has void *

@brada4

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

alpha deviates too, does it cause anything more than simple compile/cast warning?
If using netlib cblas.h helps against warning in principle OpenBLAS provides identical API and can use that 'reference' header instead.

@svillemot

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

@brada4 This is precisely what we used to do in Debian. But I recently changed it to use the implementation-specific version of the cblas.h header, hence this bug report. In the worst case, I can still roll back that change.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

Your change is correct. All that appears to be cblas can use reference header.
I can make PR but you can beat me in speed this weekend.

@claudius-hubig

This comment has been minimized.

Copy link
Author

commented Oct 7, 2017

alpha deviates too, does it cause anything more than simple compile/cast warning?

For zscal, It’s not just a warning but the same full-blown error as with the array case. Casting anything to void* is valid and, once I casted to void*, it can be cast to anything else (though not necessarily well-defined), but without a manual cast to void*, the compiler does not consider the Open(C)BLAS-function valid, while all other CBLAS implementations work just fine.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2017

@martin-frbg what do you think about making openblas.h that includes netlib cblas.h on top of own special macros/functions, but otherwise stop breaking peoples' expectations?

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2017

Not sure where a special openblas.h would come in, at least at the moment I am more inclined to change the problematic declarations in cblas.h and leave everything else in the headers as it currently is.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

maybe rectify out specific functions, then append reference header?

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2017

@claudius-hubig @sebastien-villemot I wonder if one of you could review (or at least compile-test) my PR #1347 before I let it loose onto the world ? Given how widespread the changes resulting from the requested change of the header were, I'd hate to have to self-approve it.

@claudius-hubig

This comment has been minimized.

Copy link
Author

commented Nov 18, 2017

@martin-frbg I can only comment on the changes in cblas.h, which look all good to me. However, when trying to compile my code, it appears that there is still a remaining inconsistency in the use of openblas_complex_double, for example in the cblas_zdotu_sub function. In netlib, this function takes as last parameter another void* whereas here it takes a openblas_complex_double, which seems to be defined in as a custom struct.

At least in cblas.h, only the four functions cblas_cdotu_sub, cblas_cdotc_sub, cblas_zdotu_sub and cblas_zdotc_sub seem to use this custom struct. If I change their definition to also use void* for their last parameter, my code compiles, links and runs successfully (even without recompiling the rest of OpenBLAS).

I can’t really comment on the changes in the other files, as I don’t understand them, however, they seem to be straightforward casts to fit with the lower levels of OpenBLAS?

In any case, thank you for the effort you put into maintaining this huge project!

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Nov 18, 2017

Good catch on the overlooked parameter, I will update the PR accordingly. Being late to the project has its advantages, but the potential for causing huge code breakage is definitely not one of them. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.