Skip to content

Compile cblas library as static #6347

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

Merged
merged 1 commit into from
Dec 7, 2017

Conversation

Yancey0623
Copy link
Contributor

Fixed #6309

@Yancey0623 Yancey0623 requested review from luotao1 and Xreki December 6, 2017 09:07
@tensor-tang
Copy link
Contributor

hi, @Yancey1989

MKLML do not have static lib yet, maybe you can not directly change it.

@Xreki
Copy link
Contributor

Xreki commented Dec 7, 2017

@tensor-tang I think it is OK. The cblas library is not the mklml itself, but a wrapper library to link mklml or openblas. The source code is a dummy file. It is used to record the dependency. And the CI passed too.

Copy link
Contributor

@Xreki Xreki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tensor-tang @luotao1 Please check it again.

@tensor-tang
Copy link
Contributor

tensor-tang commented Dec 7, 2017

I do not think CI can test this case, like the #6309 is not tested.

Have you ever tried locally after this change?

@tensor-tang
Copy link
Contributor

tensor-tang commented Dec 7, 2017

The source code is a dummy file. It is used to record the dependency

If it is really what you said just recording the dependency, then why changing this to static can help the issue #6309 since MKLML do not have static.

And I found this code is change from this:
2be3d32#diff-382e124cc9d4782c4a25018bc3fff9a8L76 on purpose #3461

Copy link
Contributor

@tensor-tang tensor-tang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way to solve the issue is to link the mklml.so with cblas.so, not directly change it to static.

@Yancey0623
Copy link
Contributor Author

Hi @tensor-tang

Have you ever tried locally after this change?

Yes, it passed the test.

I think the way to solve the issue is to link the mklml.so with cblas.so, not directly change it to static.

the cblas library is just a wrapper, and built from dummy.c, and it's easier to depend on OpenBlas or MKL, like:
cc_library(xxx SRCS xxx.c DEPS cblas)

and the cblas will depend MKL if WITH_MKL=ON
https://github.com/PaddlePaddle/Paddle/blob/develop/cmake/external/openblas.cmake#L128

@Yancey0623
Copy link
Contributor Author

And I found this code is change from this:
2be3d32#diff-382e124cc9d4782c4a25018bc3fff9a8L76 on purpose #3461

I saw this PR make the cblas as a dynamic library. Sorry I'm not sure why cblas must be a .so, just because mkl is a .so ( or other reason)?

@tensor-tang
Copy link
Contributor

I have tried locally.

The mkl.so is contained in the so, so the cblas.a would not bother now.

@tensor-tang tensor-tang merged commit b0f83ed into PaddlePaddle:develop Dec 7, 2017
@Yancey0623 Yancey0623 deleted the static_libcblas branch December 7, 2017 07:01
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.

3 participants