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

[openblas] Ensure we build without lapack on all platforms #6542

Merged
merged 2 commits into from
May 21, 2019

Conversation

seanwarren
Copy link
Contributor

openblas should be build without lapack (which is provided by clapack), however this was inconsistent between platforms. This PR ensures -DBUILD_WITHOUT_LAPACK=ON is set across platforms

@NancyLi1013
Copy link
Contributor

Hi @seanwarren, thanks for your PR. Could you also bump the CONTROL version?

@seanwarren
Copy link
Contributor Author

Hi @seanwarren, thanks for your PR. Could you also bump the CONTROL version?

Whoops, yes - done now.

@vicroms vicroms self-assigned this May 21, 2019
@vicroms
Copy link
Member

vicroms commented May 21, 2019

@seanwarren thanks for the PR!

@vicroms vicroms merged commit e16f9c2 into microsoft:master May 21, 2019
@seanwarren seanwarren deleted the openblas-no-lapack branch May 21, 2019 23:57
@seanwarren
Copy link
Contributor Author

Great, cheers

@cenit
Copy link
Contributor

cenit commented Jun 3, 2019

Hi, sorry but this PR made OpenBLAS not working at all on Linux. I can't find an easy way to make it work with CLapack, because OpenBLAS exports its symbols without trailing underscore, while CLapack leaves its BLAS functions (that have a trailing underscore) unresolved...
Is there any way to force gcc to add a trailing underscore to functions?
I tried to unlock OpenBLAS internal mechanism in #6730 without success for now (still trying to work on it, please check the previous PR for latest tests)...

@vicroms @seanwarren

@seanwarren
Copy link
Contributor Author

Hi @cenit - this PR was intended to fix a regression in dlib which performs detection of the trailing underscore.

Do you have a specific example of something that fails now so that I can test?

@cenit
Copy link
Contributor

cenit commented Jun 3, 2019

vcpkg install geogram

It's the first regression I saw after this PR.

@cenit
Copy link
Contributor

cenit commented Jun 3, 2019

@seanwarren while I fully appreciate the uniformity about the building, it was not nice to find the port broken, that's why I'd like some help...
in any case, I now partly fixed the geogram port, adding custom patches for OpenBLAS symbols. The problem remain intact for the CLapack symbols themselves, which are not solved by openblas! Any library depending on CLapack seems broken now on linux, since OpenBLAS cannot fulfil them (due to this underscore mismatch)

@cenit
Copy link
Contributor

cenit commented Jun 3, 2019

Am I completely missing a point?

@seanwarren
Copy link
Contributor Author

Hi @cenit, I'm sorry this introduced a regression; like everyone else ok rely on the CI to pick up errors in ports I don't use. Does the error you're seeing with geogram occur at compile or link time?

This PR wasn't about uniformity per se - the previous behaviour broke the port dlib which is an important dependency for me.

(Please excuse me explaining things I'm sure you know in the following - I just want to make sure we're on the same page) This error occured because on linux ports which depend (explicitly) on openblas and clapack trigger the installation of two versions of LAPACK - one installed by openblas and one installed by clapack. These two versions have symbols which differ by the presence of the trailing underscore. Dlib has some detection code to determine whether to use underscores or not which was confused by the presence of these two different versions, leading to a linker error.

Since many ports rely on 'interesring' custom cmake to detect BLAS and LAPACK I figured that this duplication of LAPACK would probably be causing errors elsewhere and so should be removed.

At the moment I don't fully understand how this change could cause a port to incorrectly detect BLAS, the symbols of which should remain the same.

I'm afraid that it's a bit difficult for me to investigate right now - I moved from Sydney back to Europe with my family yesterday so things are a bit hectic (I am currently typing this on my phone...)

One solution which would probably simplify things in the long run would be to have openblas install LAPACK on all platforms, remove clapack from the dependencies of other ports, and introduce a warning upon clapack installation that there is already a LAPACK install from openblas. I think that the openblas LAPACK is better maintained anyway and some detection scripts I've seen seem to assume that an openblas install will have an associated lapack version (perhaps this is the cause of this regression?). However this would obviously be quite a big change.

@cenit
Copy link
Contributor

cenit commented Jun 4, 2019

The problem is that OpenBLAS’ lapack requires FORTRAN to be built, so it’s not an option for Windows, at least without any FORTRAN compiler supported by vcpkg.

In any case now we have the problem that clapack contains unresolved symbols. Internally it calls BLAS, and somehow before the symbols were resolved and now they are not.
I will try my best to fix the problem. I am sure it was not intended.
I fully understand your situation moving again, with the family and so on. Don’t worry for here. Also I am sure I am missing a very simple point...

@seanwarren
Copy link
Contributor Author

In any case now we have the problem that clapack contains unresolved symbols. Internally it calls BLAS, and somehow before the symbols were resolved and now they are not.

Ok now I understand your earlier point. However this sounds like a problem in the clapack port, not this PR, right?

@cenit
Copy link
Contributor

cenit commented Jun 4, 2019

it is related to this pr only because before OpenBLAS resolved both BLAS and LAPACK calls under linux, because it was building its internal lapack implementation in this OS. I understand it was not clean and also maybe unreliable, so I can understand the need to uniform behaviour with other OSs.
The problem is that now CLapack has to solve lapack calls, but it becomes unable due to its internal use of BLAS calls that are unresolved. A bit of a labyrinth but i hope it's more clear now

@cenit
Copy link
Contributor

cenit commented Jun 4, 2019

the problem could also be seen from the openblas point of view. If only I was able to make it export symbols with an appended underscore, then clapack would be able to find resolution for its symbols.
But it seems that even if there are nice OPENBLAS_NEEDBUNDERSCORE and NEEDBUNDERSCORE macros, they do not work... i have to check better

@seanwarren
Copy link
Contributor Author

Ok I think I have a handle on the problem now! I'll have a look as soon as I can

@cenit
Copy link
Contributor

cenit commented Jun 4, 2019

ok fixed in the PR :)

@seanwarren
Copy link
Contributor Author

Nice one! 👍

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

Successfully merging this pull request may close these issues.

4 participants