Improve openblas CMake logic, add generic blas option. #15694
Conversation
larroy
requested review from
anirudh2290,
eric-haibin-lin and
szha
as code owners
|
Needs dmlc/mshadow#376 |
|
@marcoabreu add [pr-awaiting-review] |
|
@mxnet-label-bot add [pr-awaiting-review, build] |
There was a problem hiding this comment.
Changes LGTM!
I had another PR which had just 2 lines. Do you think adding this to your PR helps? -
https://github.com/apache/incubator-mxnet/blob/ce2b147f07d128dd919396c83d644a23fc32c61b/cmake/cmake_options.yml#L44
In that case I will close my earlier PR #15739 since it is subset of this.
|
@ChaiBapchya sure. |
|
Reminder retrigger! |
| @@ -1,34 +1,40 @@ | |||
| set(mshadow_LINKER_LIBS "") | |||
|
|
|||
| set(BLAS "Open" CACHE STRING "Selected BLAS library") | |||
| set_property(CACHE BLAS PROPERTY STRINGS "Atlas;Open;MKL") | |||
| set(BLAS "openblas" CACHE STRING "Selected BLAS library") | |||
There was a problem hiding this comment.
This breaks backwards compatibility
There was a problem hiding this comment.
It makes it consistent with Makefile, that's the whole point of the PR. Please reconsider your veto.
There was a problem hiding this comment.
None the less, it's not Backwards compatible since it changes the input arguments, right?
There was a problem hiding this comment.
Yes, you are right. The impact should be very small. Any suggestions? can we move forward?
|
|
|
There's openblas, and libblas from netlib. Two different libraries. |
|
I suggest we break this PR into two:
For 2), is there an issue filed with current approach? is any customer requesting this change? |
|
Thanks for the reviews. Unfortunately I don't have time to keep pushing this PR. |
Description
Improve CMake blas logic making it consistent and add option to link with libblas. (Netlib)