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

Remove unneeded BLAS linking for Cython modules #18777

Closed
jdemeyer opened this issue Jun 24, 2015 · 8 comments
Closed

Remove unneeded BLAS linking for Cython modules #18777

jdemeyer opened this issue Jun 24, 2015 · 8 comments

Comments

@jdemeyer
Copy link

Various Cython modules implementing matrices and vectors list

libraries = [BLAS, BLAS2]

in src/module_list.py.

However, nothing in Sage directly uses BLAS, it is only used through external libraries (linbox, numpy, ...). So it makes no sense to link to BLAS and nothing else, which means that this is a mistake.


I checked the commit history and these extensions listing only BLAS, BLAS2 as libraries go back to #3498 and #4206:

commit a8b66cc30d4aca925c1e3a1c0bb69d0f4f62fbb1
Author: Jason Grout <jason-sage@creativetrax.com>
Date:   Wed Jul 9 19:54:35 2008 -0500

    Switch the RDF and CDF matrices to a numpy 1.2 backend; factor out common functionality to matrix_double_dense.pyx.

and

commit b07600d93461c717fc6702e99afd0f2cf2eeb1df
Author: Jason Grout <jason-sage@creativetrax.com>
Date:   Tue Nov 11 03:30:38 2008 -0600

    Change the RDF/CDF vector backend to numpy, and refactor the code to a common parent vector_double_dense.  Also change names of the classes to vector_real_double_dense and vector_complex_double_dense.

It seems that the backends were changed from GSL to Numpy and I guess that the BLAS libraries (needed for GSL) were simply kept.

There might also be the mistaken assumption that BLAS was needed for numpy. It's true that numpy uses BLAS, but we use numpy using the standard Python import mechanism, it's not the usual dynamic linking (there is no -lnumpy argument needed either).

CC: @jasongrout @kiwifb @jpflori

Component: build

Author: Jeroen Demeyer

Branch/Commit: be582ec

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/18777

@jdemeyer jdemeyer added this to the sage-6.8 milestone Jun 24, 2015
@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

@jdemeyer
Copy link
Author

New commits:

be582ecRemove unneeded BLAS linking for Cython modules

@jdemeyer
Copy link
Author

Commit: be582ec

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2015

comment:4

OK I will check all of these with readelf in sage-on-gentoo. Since It is all compiled with -as-needed we'll definitely see if anything needs cblas directly. If the dependency is indirect will it break on cygwin?

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2015

Reviewer: François Bissey

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2015

comment:5

All these only link to libc and libpython which I could have seen straight away from module_list.py. We definitely want that simplification. I cannot see how we would introduce side effects by removing these linkings.

@vbraun
Copy link
Member

vbraun commented Jun 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants