You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/09/18 01:18:33 UTC
[GitHub] [incubator-mxnet] Zha0q1 opened a new pull request #19174: Make OpenBLAS use the LAPACKE apis
Zha0q1 opened a new pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174
In mxnet we wrap lapack functions in c_lapack_api.h so that we hide the differences in the underlying blas/lapack libraries such as mkl, openblas, atlas, and accelerate.
For mkl we use/wrap the LAPACKE (https://www.netlib.org/lapack/lapacke.html) c interfaces which are the cleanest. For the rest of the libraries we wrap the old CLAPACK interfaces.
Openblas is by default built with lapack support and the binary will contain a full set of LAPACKE functions. Thus this poc adds a build option with which we can use the LAPACKE apis even for openblas.
This change will make ilp64 blas/lapack support easier as now we have the same wrapping logic or both mkl and openblas with USE_LAPACKE_INTERFACE = ON. Support for ilp64 mkl #19067 will automatically mean support for ilp64 openblas.
Known issue: linux distros ship openblas binaries WITHOUT lapack so this option can only work if we build openblas from source. This is not a problem for our mxnet binary distributions as we always use our own openblas build there. However, For users building mxnet from source they must be advised against using this option unless they also build openblas from source
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-731621112
@mxnet-bot run ci [all]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 edited a comment on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 edited a comment on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-730798084
So at commit 5f6e3f2d210b98f3139346e731f13b9ef4516ec2 we were doing a omp=1 openblas build. And I only added LINK-GOMP to the builds that had -DUSE_OPENMP=OFF. For other builds that have -DUSE_OPENMP=ON static link to openblas actually worked off the bat (without setting LINK-GOMP).
Docker Image:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/Dockerfile.build.ubuntu#L98
Build Function:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/runtime_functions.sh#L317-L334
CI:
https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-cpu/detail/PR-19174/24/pipeline/44
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r530694851
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -45,6 +45,81 @@ elseif(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_BLAS_OPEN=1)
+ if(NOT MSVC)
+ # check if we need to link to omp
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep omp_get_num_threads
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_OMP_OUT
+ RESULT_VARIABLE OPENBLAS_USES_OMP_RET)
+ if(NOT OPENBLAS_USES_OMP_OUT STREQUAL "" AND NOT OPENBLAS_USES_OMP_RET)
+ message("Openblas uses OMP, automatically linking to it")
+ find_package(OpenMP)
+ if(OPENMP_FOUND)
+ message("OpenMP_CXX_LIBRARIES is ${OpenMP_CXX_LIBRARIES}")
+ list(APPEND mshadow_LINKER_LIBS "${OpenMP_CXX_LIBRARIES}")
+ else()
+ message("Trying to link to gomp")
Review comment:
currently no. I can add that in if overall the current solution looks good
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535493454
##########
File path: ci/docker/Dockerfile.build.ubuntu
##########
@@ -83,9 +82,31 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
autoconf \
gperf \
libb2-dev \
- libzstd-dev && \
+ libzstd-dev \
+ gfortran && \
rm -rf /var/lib/apt/lists/*
+# Build OpenBLAS from source
+RUN export LIBRARY_PATH=$LIBRARY_PATH:/usr/lib/gcc/x86_64-linux-gnu/7/ && \
Review comment:
Is the library path needed for clang?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527839159
##########
File path: CMakeLists.txt
##########
@@ -445,18 +449,44 @@ elseif(UNIX)
list(APPEND mxnet_LINKER_LIBS Threads::Threads)
endif()
+if(USE_ILP64_LAPACKE)
+ message("USE_ILP64_LAPACKE is ON")
+ if(NOT USE_INT64_TENSOR_SIZE)
+ message(FATAL_ERROR "USE_INT64_TENSOR_SIZE must be set to ON when USE_ILP64_LAPACKE=ON")
+ endif()
+ if(NOT USE_LAPACKE_INTERFACE)
+ message("Automatically set USE_LAPACKE_INTERFACE=ON")
+ set(USE_LAPACKE_INTERFACE ON)
+ endif()
+ add_definitions(-DMXNET_USE_ILP64_LAPACKE=1)
+endif()
+
+if(USE_LAPACKE_INTERFACE)
+ message("USE_LAPACKE_INTERFACE is ON")
+ if(NOT USE_LAPACK)
+ message("Automatically set USE_LAPACK=ON")
+ set(USE_LAPACK ON)
+ endif()
+ add_definitions(-DMXNET_USE_LAPACKE_INTERFACE=1)
+endif()
Review comment:
Because users might still want to do apt-get openblas and apt-get lapack if they do not want to build openblas from source. Also edge, mac os and windwos pipelines still use LAPACKE=0
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-730505224
@mxnet-bot run ci [unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527133490
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
+option(LINK_GOMP "Link to gomp" OFF)
Review comment:
Ubuntu offers multiple OpenBlas packages. Serial, pthread and openmp based. You can refer to https://packages.ubuntu.com/search?suite=default§ion=all&arch=any&keywords=openblas&searchon=names
I'd guess that adopting serial OpenBlas is not an option due to potential slowdown. If you think it might be ok, it would be great to run a benchmark to check before making a decision.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r529890035
##########
File path: ci/docker/Dockerfile.build.centos7
##########
@@ -70,6 +70,13 @@ RUN yum -y check-update || true && \
gcc-gfortran && \
yum clean all
+RUN mkdir ~/ninja && \
+ cd ~/ninja && \
+ wget https://github.com/ninja-build/ninja/releases/download/v1.10.1/ninja-linux.zip && \
+ unzip ninja-linux.zip -d /usr/local/bin/ && \
+ alternatives --install /usr/bin/ninja ninja /usr/local/bin/ninja 1 && \
+ ninja --version
+
Review comment:
You're introducing a dependency on cmake 3.16 and ninja 10. Both are available on Ubuntu 20.04 but not on 18.04. I just want to make sure you are conscious of that. Did you take a look at https://github.com/scikit-build/cmake-FindFortran which Brad recommended?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-737574925
Jenkins CI successfully triggered : [unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r523295383
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,7 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_LAPACKE_INTERFACE "USE LAPACKE interface for lapack support" OFF)
Review comment:
Actually there might be a reason to keep this flag. So the initial motivation to use the LAPACKE apis is that we want to support ilp64 openblas(cblas+lapacke). As I dig into the ci I think even after enabling large tensor by default we can just use ilp64 openblas on some pipelines (ubuntu cpu gpu) rather than all, because there are many different platform and build combinations to tweak if we choose otherwise. So we can use the flag to keep things how they are there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 edited a comment on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 edited a comment on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-730798084
So at commit 5f6e3f2d210b98f3139346e731f13b9ef4516ec2 we were doing a omp=1 openblas build. And I only added LINK-GOMP to the builds that had -DUSE_OPENMP=OFF. For other builds that have -DUSE_OPENMP=ON, static link to openblas actually worked off the bat (without setting LINK-GOMP).
Docker Image:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/Dockerfile.build.ubuntu#L88-L99
Build Function:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/runtime_functions.sh#L317-L334
CI:
https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-cpu/detail/PR-19174/24/pipeline/44
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-727080192
None of the jobs entered are supported.
Jobs entered by user: [macosx-x86_64]
CI supported Jobs: [windows-cpu, windows-gpu, miscellaneous, unix-cpu, unix-gpu, website, centos-cpu, centos-gpu, edge, sanity, clang]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 closed pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
Zha0q1 closed pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527974766
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -45,6 +45,26 @@ elseif(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_BLAS_OPEN=1)
+ include(CheckCSourceCompiles)
+ find_package(OpenMP COMPONENTS C)
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${OpenBLAS_INCLUDE_DIR})
+ list(APPEND CMAKE_REQUIRED_LIBRARIES ${OpenBLAS_LIB})
+ check_c_source_compiles(
+ "
+ #include <cblas.h>
+ int main(int argc, char **argv){
+ int n = 1;
+ float* A, B, C;
+ cblas_sgemm(CblasRowMajor, CblasNoTrans, CblasNoTrans, 1, 1, n, 1.0, A, n, B, 1, 0.0, C, 1);
+ return 0;
+ }
+ "
+ OPENBLAS_NO_OMP
+ )
+ if(NOT OPENBLAS_NO_OMP)
+ message("Openblas uses OMP, automatically linking to it")
+ list(APPEND mshadow_LINKER_LIBS "${OpenMP_C_LIBRARIES}")
Review comment:
Should the find_package call be in the if condition?
```suggestion
list(APPEND CMAKE_REQUIRED_INCLUDES ${OpenBLAS_INCLUDE_DIR})
list(APPEND CMAKE_REQUIRED_LIBRARIES ${OpenBLAS_LIB})
check_c_source_compiles(
"
#include <cblas.h>
int main(int argc, char **argv){
int n = 1;
float* A, B, C;
cblas_sgemm(CblasRowMajor, CblasNoTrans, CblasNoTrans, 1, 1, n, 1.0, A, n, B, 1, 0.0, C, 1);
return 0;
}
"
OPENBLAS_NO_OMP
)
if(NOT OPENBLAS_NO_OMP)
message("Openblas uses OMP, automatically linking to it")
find_package(OpenMP REQUIRED COMPONENTS C)
list(APPEND mshadow_LINKER_LIBS "${OpenMP_C_LIBRARIES}")
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r529017100
##########
File path: CMakeLists.txt
##########
@@ -444,19 +446,78 @@ elseif(UNIX)
list(APPEND mxnet_LINKER_LIBS Threads::Threads)
endif()
+if(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
Review comment:
Can we keep this configuration in the ChooseBlas file? You can use PARENT_SCOPE option to modify the cmakelists scope variables: https://cmake.org/cmake/help/latest/command/set.html
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535540001
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -45,6 +45,78 @@ elseif(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_BLAS_OPEN=1)
+ if(NOT MSVC)
+ # check if we need to link to omp
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep omp_get_num_threads
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_OMP_OUT
+ RESULT_VARIABLE OPENBLAS_USES_OMP_RET)
+ if(NOT OPENBLAS_USES_OMP_OUT STREQUAL "" AND NOT OPENBLAS_USES_OMP_RET AND NOT USE_OPENMP)
+ message("Openblas uses OMP, automatically linking to it")
+ find_package(OpenMP REQUIRED)
+ message("OpenMP_CXX_LIBRARIES is ${OpenMP_CXX_LIBRARIES}")
+ list(APPEND mshadow_LINKER_LIBS "${OpenMP_CXX_LIBRARIES}")
+ endif()
+ # check if we need to link to gfortran
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep gfortran
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_GFORTRAN_OUT
+ RESULT_VARIABLE OPENBLAS_USES_GFORTRAN_RET)
+ if(NOT OPENBLAS_USES_GFORTRAN_OUT STREQUAL "" AND NOT OPENBLAS_USES_GFORTRAN_RET)
+ message("Openblas uses GFortran, automatically linking to it")
+ file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/temp/CMakeLists.txt"
+ "cmake_minimum_required(VERSION ${CMAKE_VERSION})
+project(CheckFortran Fortran)
+set(CMAKE_Fortran_COMPILER gfortran)
+file(WRITE \"${CMAKE_CURRENT_BINARY_DIR}/temp/FortranDir.cmake\"
+\"
+set(FORTRAN_DIR \\\"\$\{CMAKE_Fortran_IMPLICIT_LINK_DIRECTORIES\}\\\")
+\")
+")
+ execute_process(
+ WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/temp/
+ COMMAND ${CMAKE_COMMAND} .
+ )
+ set(FORTRAN_DIR "")
+ include(build/temp/FortranDir.cmake)
+ find_library(FORTRAN_LIB NAMES gfortran HINTS ${FORTRAN_DIR})
+ message("FORTRAN_DIR is ${FORTRAN_DIR}")
+ message("FORTRAN_LIB is ${FORTRAN_LIB}")
+ list(APPEND mshadow_LINKER_LIBS ${FORTRAN_LIB})
+ file(REMOVE_RECURSE "${CMAKE_CURRENT_BINARY_DIR}/temp/")
+ endif()
+ # check the lapack flavor of openblas
+ include(CheckSymbolExists)
+ check_symbol_exists(OPENBLAS_USE64BITINT "${OpenBLAS_INCLUDE_DIR}/openblas_config.h" OPENBLAS_ILP64)
+ if(OPENBLAS_ILP64)
+ message("Using ILP64 OpenBLAS")
+ if(NOT USE_INT64_TENSOR_SIZE)
+ message(FATAL_ERROR "Must set USE_INT64_TENSOR_SIZE=1 when using ILP64 OpenBLAS")
+ endif()
+ else()
+ message("Using LP64 OpenBLAS")
+ endif()
+ if(EXISTS "${OpenBLAS_INCLUDE_DIR}/lapacke.h")
+ message("Detected lapacke.h, automatically using the LAPACKE interface")
+ set(USE_LAPACKE_INTERFACE ON CACHE BOOL "Use LAPACKE interface for lapack support" FORCE)
+ if(OPENBLAS_ILP64)
+ message("Automatically setting USE_ILP64_LAPACKE=1")
Review comment:
Even when supporting distro lapacke, we should be able to recognize USE_ILP64_LAPACKE automatically. We can do that later. But at this point in time, I don't see a reason for adopting USE_ILP64_LAPACKE flag (which later we'll be unable to remove for backwards compatibility)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-727101836
Jenkins CI successfully triggered : [unix-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r526538943
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
+option(LINK_GOMP "Link to gomp" OFF)
Review comment:
Ohh.. Right. I might build openblas without omp then. The apt get version of openblas is built without any omp (on ubuntu)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527255648
##########
File path: tools/dependencies/openblas.sh
##########
@@ -31,7 +31,8 @@ if [[ (! -e $DEPS_PATH/lib/libopenblas.a) ]]; then
cd $DEPS_PATH/OpenBLAS-${OPENBLAS_VERSION}
# Adding NO_DYNAMIC=1 flag causes make install to fail
- CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1
+ CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1 \
+ INTERFACE64=1 BINARY=64
Review comment:
> Also past the output of `ldd libmxnet.so` to verify that it statically links to openblas
Yeah I have confirmed the binaries with several builds. Here is one result: (unix)
```
ubuntu@ip-172-31-38-169:~/fixstaticbuild/lib$ ldd libmxnet.so
linux-vdso.so.1 (0x00007ffd3b5a0000)
libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007eff359f2000)
librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007eff357ea000)
libgfortran.so.4 => /home/ubuntu/fixstaticbuild/lib/./libgfortran.so.4 (0x00007eff3540b000)
libcudnn.so.7 => /usr/local/cuda/lib64/libcudnn.so.7 (0x00007eff1fdec000)
libnvidia-ml.so.1 => /usr/lib/x86_64-linux-gnu/libnvidia-ml.so.1 (0x00007eff1f758000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007eff1f539000)
libcudart.so.10.0 => /usr/local/cuda/lib64/libcudart.so.10.0 (0x00007eff1f2bf000)
libcufft.so.10.0 => /usr/local/cuda/lib64/libcufft.so.10.0 (0x00007eff18e0b000)
libcublas.so.10.0 => /usr/local/cuda/lib64/libcublas.so.10.0 (0x00007eff14875000)
libcusolver.so.10.0 => /usr/local/cuda/lib64/libcusolver.so.10.0 (0x00007eff0c18e000)
libcurand.so.10.0 => /usr/local/cuda/lib64/libcurand.so.10.0 (0x00007eff08027000)
libcuda.so.1 => /usr/lib/x86_64-linux-gnu/libcuda.so.1 (0x00007eff06b74000)
libnvrtc.so.10.0 => /usr/local/cuda/lib64/libnvrtc.so.10.0 (0x00007eff05558000)
libnvToolsExt.so.1 => /usr/local/cuda/lib64/libnvToolsExt.so.1 (0x00007eff0534f000)
libgomp.so.1 => /usr/lib/x86_64-linux-gnu/libgomp.so.1 (0x00007eff05120000)
libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007eff04d97000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007eff049f9000)
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007eff047e1000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007eff043f0000)
/lib64/ld-linux-x86-64.so.2 (0x00007eff6a210000)
libquadmath.so.0 => /usr/lib/x86_64-linux-gnu/libquadmath.so.0 (0x00007eff041b0000)
```
Also I used `nm -D libmxnet.so | grep blas` `nm -D libmxnet.so | grep getrf` to confirm that the symbols are hidden.
I am forcing link to `libopenblas.a` so there should be no problem wrt static linking
##########
File path: tools/dependencies/openblas.sh
##########
@@ -31,7 +31,8 @@ if [[ (! -e $DEPS_PATH/lib/libopenblas.a) ]]; then
cd $DEPS_PATH/OpenBLAS-${OPENBLAS_VERSION}
# Adding NO_DYNAMIC=1 flag causes make install to fail
- CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1
+ CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1 \
+ INTERFACE64=1 BINARY=64
Review comment:
> Also past the output of `ldd libmxnet.so` to verify that it statically links to openblas
Yeah I have confirmed the binaries with several builds. Here is one result: (ubuntu)
```
ubuntu@ip-172-31-38-169:~/fixstaticbuild/lib$ ldd libmxnet.so
linux-vdso.so.1 (0x00007ffd3b5a0000)
libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007eff359f2000)
librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007eff357ea000)
libgfortran.so.4 => /home/ubuntu/fixstaticbuild/lib/./libgfortran.so.4 (0x00007eff3540b000)
libcudnn.so.7 => /usr/local/cuda/lib64/libcudnn.so.7 (0x00007eff1fdec000)
libnvidia-ml.so.1 => /usr/lib/x86_64-linux-gnu/libnvidia-ml.so.1 (0x00007eff1f758000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007eff1f539000)
libcudart.so.10.0 => /usr/local/cuda/lib64/libcudart.so.10.0 (0x00007eff1f2bf000)
libcufft.so.10.0 => /usr/local/cuda/lib64/libcufft.so.10.0 (0x00007eff18e0b000)
libcublas.so.10.0 => /usr/local/cuda/lib64/libcublas.so.10.0 (0x00007eff14875000)
libcusolver.so.10.0 => /usr/local/cuda/lib64/libcusolver.so.10.0 (0x00007eff0c18e000)
libcurand.so.10.0 => /usr/local/cuda/lib64/libcurand.so.10.0 (0x00007eff08027000)
libcuda.so.1 => /usr/lib/x86_64-linux-gnu/libcuda.so.1 (0x00007eff06b74000)
libnvrtc.so.10.0 => /usr/local/cuda/lib64/libnvrtc.so.10.0 (0x00007eff05558000)
libnvToolsExt.so.1 => /usr/local/cuda/lib64/libnvToolsExt.so.1 (0x00007eff0534f000)
libgomp.so.1 => /usr/lib/x86_64-linux-gnu/libgomp.so.1 (0x00007eff05120000)
libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007eff04d97000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007eff049f9000)
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007eff047e1000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007eff043f0000)
/lib64/ld-linux-x86-64.so.2 (0x00007eff6a210000)
libquadmath.so.0 => /usr/lib/x86_64-linux-gnu/libquadmath.so.0 (0x00007eff041b0000)
```
Also I used `nm -D libmxnet.so | grep blas` `nm -D libmxnet.so | grep getrf` to confirm that the symbols are hidden.
I am forcing link to `libopenblas.a` so there should be no problem wrt static linking
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r522365322
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,7 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_LAPACKE_INTERFACE "USE LAPACKE interface for lapack support" OFF)
Review comment:
we can probably remove this flag. I am using this pr to test the ci for now..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-736754816
Jenkins CI successfully triggered : [sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535532323
##########
File path: ci/docker/Dockerfile.build.ubuntu
##########
@@ -83,9 +82,31 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
autoconf \
gperf \
libb2-dev \
- libzstd-dev && \
+ libzstd-dev \
+ gfortran && \
rm -rf /var/lib/apt/lists/*
+# Build OpenBLAS from source
+RUN export LIBRARY_PATH=$LIBRARY_PATH:/usr/lib/gcc/x86_64-linux-gnu/7/ && \
+ mkdir ~/openblas && \
+ cd ~/openblas && \
+ OPENBLAS_VERSION=0.3.10 && \
+ wget \
+ https://github.com/xianyi/OpenBLAS/archive/v${OPENBLAS_VERSION}.zip \
+ -O openblas.zip && \
+ unzip -q openblas.zip -d . && \
+ cd OpenBLAS-${OPENBLAS_VERSION} && \
+ CXX="clang++-6.0 -fPIC" CC="clang-6.0 -fPIC" make -j DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 \
+ USE_OPENMP=0 INTERFACE64=1 BINARY=64 && \
+ make PREFIX=/usr/local/opt install && \
Review comment:
sure I can rename the clang installation to usr/local/openblas-clang. I am a little concerned with renaming the gcc installation because we would need to hard code the blas directory in the many gcc mxnet builds whereas we only have few clang builds
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527360750
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
Review comment:
If we don't link gomp we shouldn't need gfortran. It suggests that something is wrong here. Are you referring to the CD build or CI build?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r523295383
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,7 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_LAPACKE_INTERFACE "USE LAPACKE interface for lapack support" OFF)
Review comment:
Actually there might be a need to keep this flag. So the initial motivation to use the LAPACKE apis is that we want to support ilp64 openblas(cblas+lapacke). As I dig into the ci I think even after enabling large tensor by default we can just use ilp64 openblas on some pipelines (ubuntu cpu gpu) rather than all, because there are many different platform and build combinations to tweak if we choose otherwise. So we can use the flag to keep things how they are there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-730505276
Jenkins CI successfully triggered : [unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-724422540
Jenkins CI successfully triggered : [centos-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r526538943
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
+option(LINK_GOMP "Link to gomp" OFF)
Review comment:
Ohh.. Right. I might build openblas without omp then. The apt get version of openblas is built without any omp
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-736754771
@mxnet-bot run ci [sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-732490797
asking cmake about how to find gfortran
https://gitlab.kitware.com/cmake/cmake/-/issues/21488
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu merged pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527234906
##########
File path: tools/dependencies/openblas.sh
##########
@@ -31,7 +31,8 @@ if [[ (! -e $DEPS_PATH/lib/libopenblas.a) ]]; then
cd $DEPS_PATH/OpenBLAS-${OPENBLAS_VERSION}
# Adding NO_DYNAMIC=1 flag causes make install to fail
- CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1
+ CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1 \
+ INTERFACE64=1 BINARY=64
Review comment:
Also past the output of `ldd libmxnet.so` to verify that it statically links to openblas
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r530719200
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -45,6 +45,81 @@ elseif(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_BLAS_OPEN=1)
+ if(NOT MSVC)
+ # check if we need to link to omp
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep omp_get_num_threads
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_OMP_OUT
+ RESULT_VARIABLE OPENBLAS_USES_OMP_RET)
+ if(NOT OPENBLAS_USES_OMP_OUT STREQUAL "" AND NOT OPENBLAS_USES_OMP_RET)
+ message("Openblas uses OMP, automatically linking to it")
+ find_package(OpenMP)
+ if(OPENMP_FOUND)
+ message("OpenMP_CXX_LIBRARIES is ${OpenMP_CXX_LIBRARIES}")
+ list(APPEND mshadow_LINKER_LIBS "${OpenMP_CXX_LIBRARIES}")
+ else()
Review comment:
I believe `OPENMP_FOUND` is only false on the `clang-6` build because we haven't installed the llvm openmp package in the container. Could you try adding `apt install libomp-dev` and using `find_package(OpenMP REQUIRED)` instead of `find_package(OpenMP)`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527237431
##########
File path: tools/dependencies/openblas.sh
##########
@@ -31,7 +31,8 @@ if [[ (! -e $DEPS_PATH/lib/libopenblas.a) ]]; then
cd $DEPS_PATH/OpenBLAS-${OPENBLAS_VERSION}
# Adding NO_DYNAMIC=1 flag causes make install to fail
- CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1
+ CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1 \
+ INTERFACE64=1 BINARY=64
Review comment:
Also Why is it different from here? https://github.com/apache/incubator-mxnet/pull/19174/files#diff-5ccf6e661bf0b669dd4900b71d988c969259810a3bef5c5376a212f4080186fbR98
```
CXX="g++ -fPIC" CC="gcc -fPIC" make -j DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 \
INTERFACE64=1 BINARY=64 NO_SHARED=0 NO_LAPACK=0 && \
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 edited a comment on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 edited a comment on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-730798084
So at commit 5f6e3f2d210b98f3139346e731f13b9ef4516ec2 we were doing a omp=1 openblas build. And I only added LINK-GOMP to the builds that had -DUSE_OPENMP=OFF. For other builds that have -DUSE_OPENMP=ON static link to openblas actually worked off the bat (without setting LINK-GOMP).
Docker Image:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/Dockerfile.build.ubuntu#L88-L99
Build Function:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/runtime_functions.sh#L317-L336
CI:
https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-cpu/detail/PR-19174/24/pipeline/44
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r526494159
##########
File path: ci/docker/runtime_functions.sh
##########
@@ -303,6 +305,7 @@ build_centos7_gpu() {
-DUSE_DIST_KVSTORE=ON \
-DBUILD_EXTENSION_PATH=/work/mxnet/example/extensions/lib_external_ops \
-DUSE_INT64_TENSOR_SIZE=OFF \
+ -DLINK_GFORTRAN=ON \
Review comment:
Can we have test runtime results for with and without openMP in OpenBLAS for centOS ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-731621137
Jenkins CI successfully triggered : [windows-gpu, windows-cpu, centos-gpu, miscellaneous, unix-cpu, edge, sanity, website, unix-gpu, centos-cpu, clang]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-738203267
Jenkins CI successfully triggered : [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535529625
##########
File path: ci/docker/Dockerfile.build.ubuntu
##########
@@ -83,9 +82,31 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
autoconf \
gperf \
libb2-dev \
- libzstd-dev && \
+ libzstd-dev \
+ gfortran && \
rm -rf /var/lib/apt/lists/*
+# Build OpenBLAS from source
+RUN export LIBRARY_PATH=$LIBRARY_PATH:/usr/lib/gcc/x86_64-linux-gnu/7/ && \
Review comment:
Yeah other wise it would not find gfortran
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-727080169
@mxnet-bot run ci [macosx-x86_64]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535534162
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,8 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
Review comment:
Yes I think I can hide thse two flags from users. So they can either do 1) build openblas = cblas+lapacke 2) build openblas = cblas and get distro's lapack (not lapacke). Both options are automatically detected by cmake
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 edited a comment on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 edited a comment on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-730798084
So at commit 5f6e3f2d210b98f3139346e731f13b9ef4516ec2 we were doing a omp=1 openblas build. And I only added LINK-GOMP to the builds that had -DUSE_OPENMP=OFF. For other builds that have -DUSE_OPENMP=ON, static link to openblas actually worked without setting LINK-GOMP
Docker Image:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/Dockerfile.build.ubuntu#L88-L99
Build Function:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/runtime_functions.sh#L317-L334
CI:
https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-cpu/detail/PR-19174/24/pipeline/44
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527256221
##########
File path: tools/dependencies/openblas.sh
##########
@@ -31,7 +31,8 @@ if [[ (! -e $DEPS_PATH/lib/libopenblas.a) ]]; then
cd $DEPS_PATH/OpenBLAS-${OPENBLAS_VERSION}
# Adding NO_DYNAMIC=1 flag causes make install to fail
- CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1
+ CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1 \
+ INTERFACE64=1 BINARY=64
Review comment:
> Also Why is it different from here? https://github.com/apache/incubator-mxnet/pull/19174/files#diff-5ccf6e661bf0b669dd4900b71d988c969259810a3bef5c5376a212f4080186fbR98
>
> ```
> CXX="g++ -fPIC" CC="gcc -fPIC" make -j DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 \
> INTERFACE64=1 BINARY=64 NO_SHARED=0 NO_LAPACK=0 && \
> ```
`NO_SHARED=0 NO_LAPACK=0` are default to 0 anyways. I have removed them to keep the openblas build commands consistent
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535494236
##########
File path: ci/docker/Dockerfile.build.ubuntu
##########
@@ -83,9 +82,31 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
autoconf \
gperf \
libb2-dev \
- libzstd-dev && \
+ libzstd-dev \
+ gfortran && \
rm -rf /var/lib/apt/lists/*
+# Build OpenBLAS from source
+RUN export LIBRARY_PATH=$LIBRARY_PATH:/usr/lib/gcc/x86_64-linux-gnu/7/ && \
+ mkdir ~/openblas && \
+ cd ~/openblas && \
+ OPENBLAS_VERSION=0.3.10 && \
+ wget \
+ https://github.com/xianyi/OpenBLAS/archive/v${OPENBLAS_VERSION}.zip \
+ -O openblas.zip && \
+ unzip -q openblas.zip -d . && \
+ cd OpenBLAS-${OPENBLAS_VERSION} && \
+ CXX="clang++-6.0 -fPIC" CC="clang-6.0 -fPIC" make -j DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 \
+ USE_OPENMP=0 INTERFACE64=1 BINARY=64 && \
+ make PREFIX=/usr/local/opt install && \
Review comment:
Can we use a more meaningful name than `usr/local/opt`? It will be very confusing to other developers to find two openblas installations. One in /usr/local and one in /usr/local/opt.
For example, use /usr/local/openblas-gcc and /usr/local/openblas-clang?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527276846
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
Review comment:
gfortran is a transitive dependency of gomp. This should be handled automatically
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535491060
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -45,6 +45,78 @@ elseif(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_BLAS_OPEN=1)
+ if(NOT MSVC)
+ # check if we need to link to omp
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep omp_get_num_threads
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_OMP_OUT
+ RESULT_VARIABLE OPENBLAS_USES_OMP_RET)
+ if(NOT OPENBLAS_USES_OMP_OUT STREQUAL "" AND NOT OPENBLAS_USES_OMP_RET AND NOT USE_OPENMP)
+ message("Openblas uses OMP, automatically linking to it")
+ find_package(OpenMP REQUIRED)
+ message("OpenMP_CXX_LIBRARIES is ${OpenMP_CXX_LIBRARIES}")
+ list(APPEND mshadow_LINKER_LIBS "${OpenMP_CXX_LIBRARIES}")
+ endif()
+ # check if we need to link to gfortran
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep gfortran
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_GFORTRAN_OUT
+ RESULT_VARIABLE OPENBLAS_USES_GFORTRAN_RET)
+ if(NOT OPENBLAS_USES_GFORTRAN_OUT STREQUAL "" AND NOT OPENBLAS_USES_GFORTRAN_RET)
+ message("Openblas uses GFortran, automatically linking to it")
+ file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/temp/CMakeLists.txt"
+ "cmake_minimum_required(VERSION ${CMAKE_VERSION})
+project(CheckFortran Fortran)
+set(CMAKE_Fortran_COMPILER gfortran)
+file(WRITE \"${CMAKE_CURRENT_BINARY_DIR}/temp/FortranDir.cmake\"
+\"
+set(FORTRAN_DIR \\\"\$\{CMAKE_Fortran_IMPLICIT_LINK_DIRECTORIES\}\\\")
+\")
+")
+ execute_process(
+ WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/temp/
+ COMMAND ${CMAKE_COMMAND} .
+ )
+ set(FORTRAN_DIR "")
+ include(build/temp/FortranDir.cmake)
+ find_library(FORTRAN_LIB NAMES gfortran HINTS ${FORTRAN_DIR})
+ message("FORTRAN_DIR is ${FORTRAN_DIR}")
+ message("FORTRAN_LIB is ${FORTRAN_LIB}")
+ list(APPEND mshadow_LINKER_LIBS ${FORTRAN_LIB})
+ file(REMOVE_RECURSE "${CMAKE_CURRENT_BINARY_DIR}/temp/")
+ endif()
+ # check the lapack flavor of openblas
+ include(CheckSymbolExists)
+ check_symbol_exists(OPENBLAS_USE64BITINT "${OpenBLAS_INCLUDE_DIR}/openblas_config.h" OPENBLAS_ILP64)
+ if(OPENBLAS_ILP64)
+ message("Using ILP64 OpenBLAS")
+ if(NOT USE_INT64_TENSOR_SIZE)
+ message(FATAL_ERROR "Must set USE_INT64_TENSOR_SIZE=1 when using ILP64 OpenBLAS")
+ endif()
+ else()
+ message("Using LP64 OpenBLAS")
+ endif()
+ if(EXISTS "${OpenBLAS_INCLUDE_DIR}/lapacke.h")
+ message("Detected lapacke.h, automatically using the LAPACKE interface")
+ set(USE_LAPACKE_INTERFACE ON CACHE BOOL "Use LAPACKE interface for lapack support" FORCE)
+ if(OPENBLAS_ILP64)
+ message("Automatically setting USE_ILP64_LAPACKE=1")
Review comment:
What's the use-case for manually setting `USE_ILP64_LAPACKE`? Ie. why is it exposed to the user? Should the large tensor option be already sufficient?
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,8 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
Review comment:
You're later overwriting this option. Why expose it to the user in the first place?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 closed pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 closed pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r530691220
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -45,6 +45,81 @@ elseif(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_BLAS_OPEN=1)
+ if(NOT MSVC)
+ # check if we need to link to omp
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep omp_get_num_threads
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_OMP_OUT
+ RESULT_VARIABLE OPENBLAS_USES_OMP_RET)
+ if(NOT OPENBLAS_USES_OMP_OUT STREQUAL "" AND NOT OPENBLAS_USES_OMP_RET)
+ message("Openblas uses OMP, automatically linking to it")
+ find_package(OpenMP)
+ if(OPENMP_FOUND)
+ message("OpenMP_CXX_LIBRARIES is ${OpenMP_CXX_LIBRARIES}")
+ list(APPEND mshadow_LINKER_LIBS "${OpenMP_CXX_LIBRARIES}")
+ else()
+ message("Trying to link to gomp")
Review comment:
What happens if `gomp` is not found ? Do you throw error ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-731500749
@mxnet-bot run ci [all]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r522315225
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,7 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_LAPACKE_INTERFACE "USE LAPACKE interface for lapack support" OFF)
Review comment:
What's the motivation for continuing to support `USE_LAPACKE_INTERFACE=0`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-737574853
@mxnet-bot run ci [unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-726499401
Jenkins CI successfully triggered : [centos-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-737527917
@leezu would you review thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527360957
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
Review comment:
Ok, nevermind. You are probably building openblas without omp but with the fortran based functions. I mixed those up.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-726499370
@mxnet-bot run ci [centos-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r526525538
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
+option(LINK_GOMP "Link to gomp" OFF)
Review comment:
Please be aware that gomp is just one omp implementation. For example, if user compiles with clang, llvm omp will be used. Same goes for intel compiler and iomp
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535535639
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -45,6 +45,78 @@ elseif(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_BLAS_OPEN=1)
+ if(NOT MSVC)
+ # check if we need to link to omp
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep omp_get_num_threads
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_OMP_OUT
+ RESULT_VARIABLE OPENBLAS_USES_OMP_RET)
+ if(NOT OPENBLAS_USES_OMP_OUT STREQUAL "" AND NOT OPENBLAS_USES_OMP_RET AND NOT USE_OPENMP)
+ message("Openblas uses OMP, automatically linking to it")
+ find_package(OpenMP REQUIRED)
+ message("OpenMP_CXX_LIBRARIES is ${OpenMP_CXX_LIBRARIES}")
+ list(APPEND mshadow_LINKER_LIBS "${OpenMP_CXX_LIBRARIES}")
+ endif()
+ # check if we need to link to gfortran
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep gfortran
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_GFORTRAN_OUT
+ RESULT_VARIABLE OPENBLAS_USES_GFORTRAN_RET)
+ if(NOT OPENBLAS_USES_GFORTRAN_OUT STREQUAL "" AND NOT OPENBLAS_USES_GFORTRAN_RET)
+ message("Openblas uses GFortran, automatically linking to it")
+ file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/temp/CMakeLists.txt"
+ "cmake_minimum_required(VERSION ${CMAKE_VERSION})
+project(CheckFortran Fortran)
+set(CMAKE_Fortran_COMPILER gfortran)
+file(WRITE \"${CMAKE_CURRENT_BINARY_DIR}/temp/FortranDir.cmake\"
+\"
+set(FORTRAN_DIR \\\"\$\{CMAKE_Fortran_IMPLICIT_LINK_DIRECTORIES\}\\\")
+\")
+")
+ execute_process(
+ WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/temp/
+ COMMAND ${CMAKE_COMMAND} .
+ )
+ set(FORTRAN_DIR "")
+ include(build/temp/FortranDir.cmake)
+ find_library(FORTRAN_LIB NAMES gfortran HINTS ${FORTRAN_DIR})
+ message("FORTRAN_DIR is ${FORTRAN_DIR}")
+ message("FORTRAN_LIB is ${FORTRAN_LIB}")
+ list(APPEND mshadow_LINKER_LIBS ${FORTRAN_LIB})
+ file(REMOVE_RECURSE "${CMAKE_CURRENT_BINARY_DIR}/temp/")
+ endif()
+ # check the lapack flavor of openblas
+ include(CheckSymbolExists)
+ check_symbol_exists(OPENBLAS_USE64BITINT "${OpenBLAS_INCLUDE_DIR}/openblas_config.h" OPENBLAS_ILP64)
+ if(OPENBLAS_ILP64)
+ message("Using ILP64 OpenBLAS")
+ if(NOT USE_INT64_TENSOR_SIZE)
+ message(FATAL_ERROR "Must set USE_INT64_TENSOR_SIZE=1 when using ILP64 OpenBLAS")
+ endif()
+ else()
+ message("Using LP64 OpenBLAS")
+ endif()
+ if(EXISTS "${OpenBLAS_INCLUDE_DIR}/lapacke.h")
+ message("Detected lapacke.h, automatically using the LAPACKE interface")
+ set(USE_LAPACKE_INTERFACE ON CACHE BOOL "Use LAPACKE interface for lapack support" FORCE)
+ if(OPENBLAS_ILP64)
+ message("Automatically setting USE_ILP64_LAPACKE=1")
Review comment:
Yes if openblas.so include both cblas and lapacke then we can automatically detect. I originally though we could support distro's lapacke as well but that might over-complicate things..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-724422505
@mxnet-bot run ci [centos-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535535639
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -45,6 +45,78 @@ elseif(BLAS STREQUAL "Open" OR BLAS STREQUAL "open")
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_BLAS_OPEN=1)
+ if(NOT MSVC)
+ # check if we need to link to omp
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep omp_get_num_threads
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_OMP_OUT
+ RESULT_VARIABLE OPENBLAS_USES_OMP_RET)
+ if(NOT OPENBLAS_USES_OMP_OUT STREQUAL "" AND NOT OPENBLAS_USES_OMP_RET AND NOT USE_OPENMP)
+ message("Openblas uses OMP, automatically linking to it")
+ find_package(OpenMP REQUIRED)
+ message("OpenMP_CXX_LIBRARIES is ${OpenMP_CXX_LIBRARIES}")
+ list(APPEND mshadow_LINKER_LIBS "${OpenMP_CXX_LIBRARIES}")
+ endif()
+ # check if we need to link to gfortran
+ execute_process(COMMAND ${CMAKE_NM} -g ${OpenBLAS_LIB}
+ COMMAND grep gfortran
+ WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+ OUTPUT_VARIABLE OPENBLAS_USES_GFORTRAN_OUT
+ RESULT_VARIABLE OPENBLAS_USES_GFORTRAN_RET)
+ if(NOT OPENBLAS_USES_GFORTRAN_OUT STREQUAL "" AND NOT OPENBLAS_USES_GFORTRAN_RET)
+ message("Openblas uses GFortran, automatically linking to it")
+ file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/temp/CMakeLists.txt"
+ "cmake_minimum_required(VERSION ${CMAKE_VERSION})
+project(CheckFortran Fortran)
+set(CMAKE_Fortran_COMPILER gfortran)
+file(WRITE \"${CMAKE_CURRENT_BINARY_DIR}/temp/FortranDir.cmake\"
+\"
+set(FORTRAN_DIR \\\"\$\{CMAKE_Fortran_IMPLICIT_LINK_DIRECTORIES\}\\\")
+\")
+")
+ execute_process(
+ WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/temp/
+ COMMAND ${CMAKE_COMMAND} .
+ )
+ set(FORTRAN_DIR "")
+ include(build/temp/FortranDir.cmake)
+ find_library(FORTRAN_LIB NAMES gfortran HINTS ${FORTRAN_DIR})
+ message("FORTRAN_DIR is ${FORTRAN_DIR}")
+ message("FORTRAN_LIB is ${FORTRAN_LIB}")
+ list(APPEND mshadow_LINKER_LIBS ${FORTRAN_LIB})
+ file(REMOVE_RECURSE "${CMAKE_CURRENT_BINARY_DIR}/temp/")
+ endif()
+ # check the lapack flavor of openblas
+ include(CheckSymbolExists)
+ check_symbol_exists(OPENBLAS_USE64BITINT "${OpenBLAS_INCLUDE_DIR}/openblas_config.h" OPENBLAS_ILP64)
+ if(OPENBLAS_ILP64)
+ message("Using ILP64 OpenBLAS")
+ if(NOT USE_INT64_TENSOR_SIZE)
+ message(FATAL_ERROR "Must set USE_INT64_TENSOR_SIZE=1 when using ILP64 OpenBLAS")
+ endif()
+ else()
+ message("Using LP64 OpenBLAS")
+ endif()
+ if(EXISTS "${OpenBLAS_INCLUDE_DIR}/lapacke.h")
+ message("Detected lapacke.h, automatically using the LAPACKE interface")
+ set(USE_LAPACKE_INTERFACE ON CACHE BOOL "Use LAPACKE interface for lapack support" FORCE)
+ if(OPENBLAS_ILP64)
+ message("Automatically setting USE_ILP64_LAPACKE=1")
Review comment:
Yes if openblas.so include both cblas and lapacke then we can automatically detect. I originally thought we could support distro's lapacke as well but that might over-complicate things..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527360957
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
Review comment:
Ok, nevermind. You are probably building openblas without omp but with the fortran based functions. I mixed those up. We should still autodetect this though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535532323
##########
File path: ci/docker/Dockerfile.build.ubuntu
##########
@@ -83,9 +82,31 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
autoconf \
gperf \
libb2-dev \
- libzstd-dev && \
+ libzstd-dev \
+ gfortran && \
rm -rf /var/lib/apt/lists/*
+# Build OpenBLAS from source
+RUN export LIBRARY_PATH=$LIBRARY_PATH:/usr/lib/gcc/x86_64-linux-gnu/7/ && \
+ mkdir ~/openblas && \
+ cd ~/openblas && \
+ OPENBLAS_VERSION=0.3.10 && \
+ wget \
+ https://github.com/xianyi/OpenBLAS/archive/v${OPENBLAS_VERSION}.zip \
+ -O openblas.zip && \
+ unzip -q openblas.zip -d . && \
+ cd OpenBLAS-${OPENBLAS_VERSION} && \
+ CXX="clang++-6.0 -fPIC" CC="clang-6.0 -fPIC" make -j DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 \
+ USE_OPENMP=0 INTERFACE64=1 BINARY=64 && \
+ make PREFIX=/usr/local/opt install && \
Review comment:
sure I can rename the clang installation to usr/local/openblas-clang. I am a little concerned with renaming the gcc installation because we would need to hard code the blas directory in the many gcc mxnet build whereas we only have few clang builds
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-728358368
Jenkins CI successfully triggered : [unix-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527255648
##########
File path: tools/dependencies/openblas.sh
##########
@@ -31,7 +31,8 @@ if [[ (! -e $DEPS_PATH/lib/libopenblas.a) ]]; then
cd $DEPS_PATH/OpenBLAS-${OPENBLAS_VERSION}
# Adding NO_DYNAMIC=1 flag causes make install to fail
- CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1
+ CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1 \
+ INTERFACE64=1 BINARY=64
Review comment:
> Also past the output of `ldd libmxnet.so` to verify that it statically links to openblas
Yeah I have confirmed the binaries with several builds. Here is one result:
```
ubuntu@ip-172-31-38-169:~/fixstaticbuild/lib$ ldd libmxnet.so
linux-vdso.so.1 (0x00007ffd3b5a0000)
libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007eff359f2000)
librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007eff357ea000)
libgfortran.so.4 => /home/ubuntu/fixstaticbuild/lib/./libgfortran.so.4 (0x00007eff3540b000)
libcudnn.so.7 => /usr/local/cuda/lib64/libcudnn.so.7 (0x00007eff1fdec000)
libnvidia-ml.so.1 => /usr/lib/x86_64-linux-gnu/libnvidia-ml.so.1 (0x00007eff1f758000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007eff1f539000)
libcudart.so.10.0 => /usr/local/cuda/lib64/libcudart.so.10.0 (0x00007eff1f2bf000)
libcufft.so.10.0 => /usr/local/cuda/lib64/libcufft.so.10.0 (0x00007eff18e0b000)
libcublas.so.10.0 => /usr/local/cuda/lib64/libcublas.so.10.0 (0x00007eff14875000)
libcusolver.so.10.0 => /usr/local/cuda/lib64/libcusolver.so.10.0 (0x00007eff0c18e000)
libcurand.so.10.0 => /usr/local/cuda/lib64/libcurand.so.10.0 (0x00007eff08027000)
libcuda.so.1 => /usr/lib/x86_64-linux-gnu/libcuda.so.1 (0x00007eff06b74000)
libnvrtc.so.10.0 => /usr/local/cuda/lib64/libnvrtc.so.10.0 (0x00007eff05558000)
libnvToolsExt.so.1 => /usr/local/cuda/lib64/libnvToolsExt.so.1 (0x00007eff0534f000)
libgomp.so.1 => /usr/lib/x86_64-linux-gnu/libgomp.so.1 (0x00007eff05120000)
libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007eff04d97000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007eff049f9000)
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007eff047e1000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007eff043f0000)
/lib64/ld-linux-x86-64.so.2 (0x00007eff6a210000)
libquadmath.so.0 => /usr/lib/x86_64-linux-gnu/libquadmath.so.0 (0x00007eff041b0000)
```
Also I used `nm -D libmxnet.so | grep blas` `nm -D libmxnet.so | grep getrf` to confirm that the symbols are hidden.
I am forcing link to `libopenblas.a` so there should be no problem wrt static linking
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 closed pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 closed pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-727101824
@mxnet-bot run ci [unix-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-728358251
@mxnet-bot run ci [unix-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-738203105
@mxnet-bot run ci [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527237431
##########
File path: tools/dependencies/openblas.sh
##########
@@ -31,7 +31,8 @@ if [[ (! -e $DEPS_PATH/lib/libopenblas.a) ]]; then
cd $DEPS_PATH/OpenBLAS-${OPENBLAS_VERSION}
# Adding NO_DYNAMIC=1 flag causes make install to fail
- CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1
+ CFLAGS="-fPIC" CXXFLAGS="-fPIC" $MAKE DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 USE_OPENMP=1 \
+ INTERFACE64=1 BINARY=64
Review comment:
Also Why is it different from here? https://github.com/apache/incubator-mxnet/pull/19174/files#diff-5ccf6e661bf0b669dd4900b71d988c969259810a3bef5c5376a212f4080186fbR98
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19174: Make OpenBLAS use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-694591765
Hey @Zha0q1 , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [unix-gpu, windows-cpu, unix-cpu, website, edge, centos-cpu, centos-gpu, clang, sanity, miscellaneous, windows-gpu]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r529890035
##########
File path: ci/docker/Dockerfile.build.centos7
##########
@@ -70,6 +70,13 @@ RUN yum -y check-update || true && \
gcc-gfortran && \
yum clean all
+RUN mkdir ~/ninja && \
+ cd ~/ninja && \
+ wget https://github.com/ninja-build/ninja/releases/download/v1.10.1/ninja-linux.zip && \
+ unzip ninja-linux.zip -d /usr/local/bin/ && \
+ alternatives --install /usr/bin/ninja ninja /usr/local/bin/ninja 1 && \
+ ninja --version
+
Review comment:
You're introducing a dependency on cmake 3.16 and ninja 10. Both are available on Ubuntu 20.04 but not on 18.04. We can do it in principle, but it would cause some trouble for users on older systems. I just want to make sure you are conscious of that. Did you take a look at https://github.com/scikit-build/cmake-FindFortran which Brad recommended?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 closed pull request #19174: [poc] Add option for OpenBLAS to use the LAPACKE apis
Posted by GitBox <gi...@apache.org>.
Zha0q1 closed pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527346074
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
Review comment:
This flag is used on Centos. On Centos I tried a omp=1 openblas build and one of the testcase would just hang until time-out. Building openblas with omp=0 resolved the issue. So there since we are not linking gomp we sort of need this flag.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527346074
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
Review comment:
This flag is used on Centos. On Centos I tried a omp=1 openblas build and I think gomp clashed with mxnet llvm omp and one of the testcase would just hang until time-out. Building openblas with omp=0 resolved the issue. So there since we are not linking gomp we sort of need this flag.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-730798084
So at commit 5f6e3f2d210b98f3139346e731f13b9ef4516ec2 we were doing a omp=1 openblas build. And I only added LINK-GOMP to the builds that had -DUSE_OPENMP=OFF. For other builds that have -DUSE_OPENMP=ON static link to openblas actually worked off the bat (without setting LINK-GOMP).
Docker Image:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/Dockerfile.build.ubuntu#L98
Build Function:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/runtime_functions.sh#L317
CI:
https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-cpu/detail/PR-19174/24/pipeline/44
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 closed pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 closed pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 edited a comment on pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 edited a comment on pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#issuecomment-730798084
So at commit 5f6e3f2d210b98f3139346e731f13b9ef4516ec2 we were doing a omp=1 openblas build. And I only added LINK-GOMP to the builds that had -DUSE_OPENMP=OFF. For other builds that have -DUSE_OPENMP=ON static link to openblas actually worked off the bat (without setting LINK-GOMP).
Docker Image:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/Dockerfile.build.ubuntu#L88-L99
Build Function:
https://github.com/apache/incubator-mxnet/blob/5f6e3f2d210b98f3139346e731f13b9ef4516ec2/ci/docker/runtime_functions.sh#L317-L334
CI:
https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-cpu/detail/PR-19174/24/pipeline/44
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19174: [wip] Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r527363911
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
+option(LINK_LAPACK "Link to lapack. Use this option if lapack is not provided with the BLAS lib" OFF)
Review comment:
We should be able to use https://cmake.org/cmake/help/latest/module/CheckSymbolExists.html or https://cmake.org/cmake/help/latest/module/CheckCSourceCompiles.html#module:CheckCSourceCompiles to detect this
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
Review comment:
We should be able to use https://cmake.org/cmake/help/latest/module/CheckSymbolExists.html or https://cmake.org/cmake/help/latest/module/CheckCSourceCompiles.html#module:CheckCSourceCompiles to detect this
##########
File path: CMakeLists.txt
##########
@@ -445,18 +449,44 @@ elseif(UNIX)
list(APPEND mxnet_LINKER_LIBS Threads::Threads)
endif()
+if(USE_ILP64_LAPACKE)
+ message("USE_ILP64_LAPACKE is ON")
+ if(NOT USE_INT64_TENSOR_SIZE)
+ message(FATAL_ERROR "USE_INT64_TENSOR_SIZE must be set to ON when USE_ILP64_LAPACKE=ON")
+ endif()
+ if(NOT USE_LAPACKE_INTERFACE)
+ message("Automatically set USE_LAPACKE_INTERFACE=ON")
+ set(USE_LAPACKE_INTERFACE ON)
+ endif()
+ add_definitions(-DMXNET_USE_ILP64_LAPACKE=1)
+endif()
+
+if(USE_LAPACKE_INTERFACE)
+ message("USE_LAPACKE_INTERFACE is ON")
+ if(NOT USE_LAPACK)
+ message("Automatically set USE_LAPACK=ON")
+ set(USE_LAPACK ON)
+ endif()
+ add_definitions(-DMXNET_USE_LAPACKE_INTERFACE=1)
+endif()
Review comment:
Why do we need to support LAPACKE=0 following this PR?
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,10 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_ILP64_LAPACKE "Use ILP64 LAPACKE interface" OFF)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
+option(LINK_GFORTRAN "Link to gfortran" OFF)
Review comment:
We should be able to use https://cmake.org/cmake/help/latest/module/CheckSymbolExists.html or https://cmake.org/cmake/help/latest/module/CheckCSourceCompiles.html#module:CheckCSourceCompiles to detect this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19174: Force static link to openblas on ci + add lapacke option for lapack
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19174:
URL: https://github.com/apache/incubator-mxnet/pull/19174#discussion_r535534162
##########
File path: CMakeLists.txt
##########
@@ -60,6 +60,8 @@ cmake_dependent_option(USE_SSE "Build with x86 SSE instruction support" ON
"CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64" OFF)
option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
option(USE_LAPACK "Build with lapack support" ON)
+option(USE_LAPACKE_INTERFACE "Use LAPACKE interface for lapack support" OFF)
Review comment:
I think I can hide this from users. So they can either do 1) build openblas = cblas+lapacke 2) build openblas = cblas and get distro's lapack (not lapacke). Both options are automatically detected by cmake
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org