Skip to content

Compile cblas library as static#6347

Merged
tensor-tang merged 1 commit intoPaddlePaddle:developfrom
Yancey0623:static_libcblas
Dec 7, 2017
Merged

Compile cblas library as static#6347
tensor-tang merged 1 commit intoPaddlePaddle:developfrom
Yancey0623:static_libcblas

Conversation

@Yancey0623
Copy link
Contributor

Fixed #6309

@Yancey0623 Yancey0623 requested review from Xreki and luotao1 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

3 participants