Closed dane-unltd closed 9 years ago
So, it does not work at the moment. go install is fine, but go test or building other executables which import cblas fails because of the relative path in the LDFLAGs. Need to figure out how to use an absolut path in the cgo comment based on the GOPATH environment variable.
I removed the LDFLAGS from the cblas/blas.go file. I think it is preferable this way because changing the source breaks go get -u on the blas package. What do you think?
What has this been tested on?
BTW Changes should be made to genCode.pl
rather than blas.go
.
It works on my MacBook with openblas and the system libraries on OS X 10.9 and I also tested it on a linux machine with both openblas and the intel MKL.
I am not sure this approach is optimal, because you have to specify the environment variable every time you want to test/build the cblas package.
We could also remove blas.go from the repository and let people run the generator with an argument specifying the LDFLAGS, but windows people would probably not like that.
It would be interesting to know how people actually use the package. Maybe I am also overthinking this ...
I think a reasonable default is the appropriate way to deal with this.
For me, the current darwin flag works out of the box (the accelerate framework, which I believe is the officially supported one). It might be nice to have a "canonical" installation procedure for a better blas. Do either ATLAS or OpenBLAS have canonical locations, or could we have people put it in $GOPATH/github.com/gonum/blas/cblascode or something?
Another option is to have the user define a CBLAS_LIB environment variable, and then have them run the autogen script (which could be changed to use such a variable)
Also, just so people are aware, I believe OpenBLAS has a bug in the drotmg function.
What would be in $GOPATH/github.com/gonum/blas/cblascode
?
I don't like the idea of an env var. There should be a reasonable default that we know passes tests (we don't have that yet, but it will come). This way when things fail, we can know that it is due to us and not some client configuration.
BTW, on x86-64 Fedora 20 libcblas.so
lives in /usr/lib64/atlas/
. AFAICT, we can just add -L/usr/lib64/atlas
to default ldflags for this but I wonder why Go doesn't pick it up from ldconfig path...
Copied from gonum-dev...
I've put together a docker repository that builds an ubuntu container with Go, OpenBLAS, and bindings for gonum/lapack at https://registry.hub.docker.com/u/jonlawlor/gonum-lapack/
If you have docker installed then you can try it out with
docker run -t -i jonlawlor/gonum-lapack:1.3.1 /bin/bash
which will drop you into a shell.
Currently it successfully binds gonum/lapack/clapack but gonum/blas/cblas doesn't work correctly due to this issue.
Maybe blas could use the same technique as the lapack package (the perl script)? Or maybe an environment variable. There may also be a hybrid approach, as all go installations will have a$GOPATH
set - maybe placing files or links somewhere in that would work.
Those changes work for me. With that approach we could probably get rid of the perl scripts as well.
Can you explain further? genBlas.pl does more than just organising linking information.=
It seems like it should be possible to split apart the cblas.go and clapack.go files output by the gen*.pl into several packages with each conditional part of the files. For example, the excludeComplex flag could be removed, and there could be a cblasreal.go and cblascomplex.go, where each implements that portion of the blas.
Then again, I could always be missing something.
My intention is to rewrite the code generation from a table of function descriptions structs that will be compiled by a genBlas.pl derivative. This change is something that is most likely going to be forced on us by changes to the GC since using templates is much more maintainable than munging C->Go. I don't have time to do that right now though. I don't really see that there is much point doing more than keeping genBlas.pl working. Someone else can do the genLapack.pl transformation whenever they want. There probably needs to be an issue and some discussion about we are going to deal with the upcoming GC issues and what they will do to the API here, but we can't make any real decisions yet since the GC changes have not yet been formally proposed.
Is there anything that prevents merging this PR?
LGTM. @kortschak?
The comment for OSX should be to use the accelerate framework, not vecilb. Veclib is deprecated.
Why remove the linker flag for Accelerate on OSX? It works out of the box with no need to install or edit anything, and so is by far the easiest thing on mac.
LGTM after addressing @btravey's commments.
If someone wanted to use another blas library, would that be possible if "#cgo darwin LDFLAGS: -framework Accelerate" were added to genBlas.pl/blas.go? According to cgo docs:
When building, the CGO_CFLAGS, CGO_CPPFLAGS, CGO_CXXFLAGS and CGO_LDFLAGS environment variables are added to the flags derived from these directives.
So, what would happen if the linker got "-framework Accelerate -lopenblas"? Error about multiple definition? I can't check myself.
Another question: can colMajor constant be removed from genBlas.pl?
Another question: can colMajor constant be removed from genBlas.pl?
Yes.
I'll get to the rest soon - finishing up competing work for the year.
If someone wanted to use something else, they would have to edit the blas.go file, which they have to do anyway under these changes if I understand correctly.
On Dec 2, 2014, at 4:55 PM, Vladimír Chalupecký notifications@github.com wrote:
If someone wanted to use another blas library, would that be possible if "#cgo darwin LDFLAGS: -framework Accelerate" were added to genBlas.pl/blas.go? According to cgo docs:
When building, the CGO_CFLAGS, CGO_CPPFLAGS, CGO_CXXFLAGS and CGO_LDFLAGS environment variables are added to the flags derived from these directives.
So, what would happen if the linker got "-framework Accelerate -lopenblas"? Error about multiple definition? I can't check myself.
Another question: can colMajor constant be removed from genBlas.pl?
— Reply to this email directly or view it on GitHub.
As is this PR now, everyone has to set LDFLAGS environment variable. If Accelerate were added to genBlas, those (few?) who would want to use something else would have to edit blas.go (and have their gonum/blas working tree either dirty or on a non-master branch and merge from time to time). Is that acceptable?
Is that acceptable?
No.
Sorry, I guess I misunderstood. The way this works now, every time someone wants to install/ test cblas they have to run install with the CGO_LDFLAGS environment variable set, is that correct?
Yes, everyone has to set CGO_LDFLAGS (I made a mistake above).
LGTM
Is it ok to delete cblas-info branch, @dane-unltd, @kortschak? It contains only one commit that is better handled by this (closed) PR.
SGTM
I just added some lines to the installation instructions and changed the LDFLAGS such that it works out of the box with the recommended installation procedure. Maybe someone could test it on another machine.