Demostration of cmake refine for HIP support. #9165

sabreshao · 2018-03-16T09:52:15Z

Add option WITH_AMD_GPU.
Add cmake/hip.cmake for HIP toolchain.
Some external module such as eigen may need HIP port.
Add macro hip_library/hip_binary/hip_test to cmake/generic.cmake.
Add one HIP source concat.hip.cu as an example. Each .cu may have its corresponding .hip.cu.

1. Add option WITH_AMD_GPU. 2. Add cmake/hip.cmake for HIP toolchain. 3. Some external module such as eigen may need HIP port. 4. Add macro hip_library/hip_binary/hip_test to cmake/generic.cmake. 5. Add one HIP source concat.hip.cu as an example. Each .cu may have its corresponding .hip.cu.

CLAassistant · 2018-03-16T09:52:22Z

All committers have signed the CLA.

luotao1 · 2018-03-16T12:30:11Z

CMakeLists.txt

@@ -69,6 +70,9 @@ if(NOT CMAKE_BUILD_TYPE)
      FORCE)
 endif()

+if(WITH_AMD_GPU)
+endif()


line 73-74 could be removed since there is nothing between them.

luotao1 · 2018-03-16T12:34:03Z

cmake/external/eigen.cmake

-    INSTALL_COMMAND   ""
-    TEST_COMMAND      ""
-)
+INCLUDE_DIRECTORIES(${EIGEN_SOURCE_DIR}/src/extern_eigen3)


if(WITH_AMD_GPU) SET(GIT_REPOSITORY "https://github.com/sabreshao/hipeigen.git") SET(GIT_TAG 0cba03ff9f8f9f70bbd92ac5857b031aa8fed6f9) else() SET(GIT_REPOSITORY "https://github.com/RLovelett/eigen.git") SET(GIT_TAG 0cba03ff9f8f9f70bbd92ac5857b031aa8fed6f9) endif() ExternalProject_Add( extern_eigen3 ${EXTERNAL_PROJECT_LOG_ARGS} GIT_REPOSITORY ${GIT_REPOSITORY} GIT_TAG ${GIT_TAG} ... )

may be more cleaner

In AMD GPU build, use hipeigen instead since it includes necessary patches for HIP.

luotao1 · 2018-03-16T12:38:22Z

cmake/hip.cmake

+include_directories("/opt/rocm/hiprand/include")
+include_directories("/opt/rocm/rocrand/include")
+include_directories("/opt/rocm/rccl/include")
+include_directories("/opt/rocm/thrust")


Could /opt/rocm/include etc be a relative path like ${CUDA_INCLUDE_DIRS}?

Until now no such thing is defined in HIP/ROCm.

No, we don't have such define. NV defines /usr/local/cuda as the symbolic link, that's why they need those env vars so users can define those to point to the absolute CUDA paths.

luotao1 · 2018-03-16T12:39:25Z

cmake/hip.cmake

+    list(APPEND HIP_HCC_FLAGS  ${CMAKE_CXX_FLAGS_DEBUG})
+elseif(CMAKE_BUILD_TYPE  STREQUAL "Release")
+# Disable optimization since one eigen symbol will be removed in math_function.cu
+    #list(APPEND HIP_HCC_FLAGS  ${CMAKE_CXX_FLAGS_RELEASE})


line 31-32 could be removed if not necessary.

luotao1 · 2018-03-16T12:41:41Z

paddle/fluid/operators/CMakeLists.txt

@@ -76,6 +76,9 @@ function(op_library TARGET)
    if (WITH_GPU)
        nv_library(${TARGET} SRCS ${cc_srcs} ${cu_cc_srcs} ${cudnn_cu_cc_srcs} ${mkldnn_cc_srcs} ${cu_srcs} DEPS ${op_library_DEPS}
                ${op_common_deps})
+    elseif (WITH_AMD_GPU)
+        hip_library(${TARGET} SRCS ${cc_srcs} ${hip_cc_srcs} ${miopen_cu_cc_srcs} ${mkldnn_cc_srcs} ${hip_srcs} DEPS
+                ${op_library_DEPS} ${op_common_deps})


Where are the definitions of ${hip_cc_srcs}, ${miopen_cu_cc_srcs} and ${hip_srcs}?

Refined paddle/fluid/operators/CMakeLists.txt .

luotao1 · 2018-03-16T12:45:59Z

paddle/fluid/pybind/CMakeLists.txt

@@ -1,9 +1,16 @@
 if(WITH_PYTHON)


What's the purpose of updating this pybind/CMakeLists.txt?

The reason of updating pybind/CMakeLists.txt is that shared library which refers any HIP kernel must be linked with hipcc rather than gcc.

1. Add option WITH_AMD_GPU. 2. Add cmake/hip.cmake for HIP toolchain. 3. Some external module such as eigen may need HIP port. 4. Add macro hip_library/hip_binary/hip_test to cmake/generic.cmake. 5. Add one HIP source concat.hip.cu as an example. Each .cu may have its corresponding .hip.cu.

reyoung

LGTM. There are some tiny comments. However, do not let them block this PR.

reyoung · 2018-03-21T08:22:30Z

cmake/hip.cmake

+include_directories("/opt/rocm/rccl/include")
+include_directories("/opt/rocm/thrust")
+
+list(APPEND EXTERNAL_LIBS "-L/opt/rocm/lib/ -lhip_hcc")


There are too many hard codes in this file. Maybe we can modify the FindHip.cmake after this PR?

reyoung · 2018-03-21T08:23:07Z

cmake/hip.cmake

+
+list(APPEND EXTERNAL_LIBS "-L/opt/rocm/lib/ -lhip_hcc")
+
+set(HIP_HCC_FLAGS "${HIP_HCC_FLAGS} -fPIC -DPADDLE_WITH_HIP -std=c++14" )


Should we use c++14 by default? Paddle is currently using c++11

@sabreshao , could you check the c++14 option here? This is not required by HIP as well.

@sunway513 @reyoung c++14 is to enable "function return type deduction in lambda", which may be needed in following PR.

helinwang · 2018-03-21T20:02:58Z

@sabreshao thanks for the PR!

The CI seems to fail, can you take a look? (PR can not be merged if CI fails)

[02:47:35] :     [Step 1/1]   2/215 Test #209: test_recognize_digits ...........................***Failed    0.70 sec
[02:47:35] :     [Step 1/1] Traceback (most recent call last):
[02:47:35] :     [Step 1/1]   File "test_recognize_digits.py", line 16, in <module>
[02:47:35] :     [Step 1/1]     import paddle.fluid as fluid
[02:47:35] :     [Step 1/1]   File "/paddle/build/python/build/lib-python/paddle/fluid/__init__.py", line 17, in <module>
[02:47:35] :     [Step 1/1]     import framework
[02:47:35] :     [Step 1/1]   File "/paddle/build/python/build/lib-python/paddle/fluid/framework.py", line 22, in <module>
[02:47:35] :     [Step 1/1]     from . import core
[02:47:35] :     [Step 1/1] ImportError: /paddle/build/python/build/lib-python/paddle/fluid/core.so: undefined symbol: _ZN6paddle6pybind18BindRecordIOWriterERN8pybind116moduleE
[02:47:35] :     [Step 1/1]
[02:47:35] :     [Step 1/1]         Start 210: test_word2vec
[02:47:41] :     [Step 1/1]   3/215 Test #206: test_image_classification .......................***Failed    7.25 sec
[02:47:41] :     [Step 1/1] Traceback (most recent call last):
[02:47:41] :     [Step 1/1]   File "test_image_classification.py", line 18, in <module>
[02:47:41] :     [Step 1/1]     import paddle.fluid as fluid
[02:47:41] :     [Step 1/1]   File "/paddle/build/python/build/lib-python/paddle/fluid/__init__.py", line 17, in <module>
[02:47:41] :     [Step 1/1]     import framework
[02:47:41] :     [Step 1/1]   File "/paddle/build/python/build/lib-python/paddle/fluid/framework.py", line 22, in <module>
[02:47:41] :     [Step 1/1]     from . import core
[02:47:41] :     [Step 1/1] ImportError: /paddle/build/python/build/lib-python/paddle/fluid/core.so: undefined symbol: _ZN6paddle6pybind18BindRecordIOWriterERN8pybind116moduleE
[02:47:41] :     [Step 1/1]
[02:47:41] :     [Step 1/1]         Start   1: serialization_test
[02:47:41] :     [Step 1/1]   4/215 Test #208: test_label_semantic_roles .......................***Failed    7.25 sec
[02:47:41] :     [Step 1/1] Traceback (most recent call last):
[02:47:41] :     [Step 1/1]   File "test_label_semantic_roles.py", line 20, in <module>
[02:47:41] :     [Step 1/1]     import paddle.fluid as fluid
[02:47:41] :     [Step 1/1]   File "/paddle/build/python/build/lib-python/paddle/fluid/__init__.py", line 17, in <module>
[02:47:41] :     [Step 1/1]     import framework
[02:47:41] :     [Step 1/1]   File "/paddle/build/python/build/lib-python/paddle/fluid/framework.py", line 22, in <module>
[02:47:41] :     [Step 1/1]     from . import core
[02:47:41] :     [Step 1/1] ImportError: /paddle/build/python/build/lib-python/paddle/fluid/core.so: undefined symbol: _ZN6paddle6pybind18BindRecordIOWriterERN8pybind116moduleE

Fix CI.

reyoung

Excellent!

qingqing01 requested review from reyoung, helinwang, jacquesqiao, kuke, luotao1, qingqing01 and tonyyang-svail March 16, 2018 10:51

luotao1 added the AMD label Mar 16, 2018

luotao1 reviewed Mar 16, 2018

View reviewed changes

reyoung previously approved these changes Mar 21, 2018

View reviewed changes

CMake refine for HIP support.

e0ac6bc

Fix CI.

sabreshao dismissed reyoung’s stale review via e0ac6bc March 22, 2018 02:49

reyoung approved these changes Mar 22, 2018

View reviewed changes

luotao1 merged commit 9126e62 into PaddlePaddle:develop Mar 22, 2018

sabreshao deleted the amd_cmake_01 branch August 10, 2018 02:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demostration of cmake refine for HIP support. #9165

Demostration of cmake refine for HIP support. #9165

sabreshao commented Mar 16, 2018

CLAassistant commented Mar 16, 2018 •

edited

Loading

luotao1 Mar 16, 2018

sabreshao Mar 20, 2018

luotao1 Mar 16, 2018

sabreshao Mar 20, 2018

luotao1 Mar 16, 2018

sabreshao Mar 20, 2018

sunway513 Mar 21, 2018

luotao1 Mar 16, 2018

sabreshao Mar 20, 2018

luotao1 Mar 16, 2018

sabreshao Mar 20, 2018

luotao1 Mar 16, 2018

sabreshao Mar 20, 2018

reyoung left a comment

reyoung Mar 21, 2018

reyoung Mar 21, 2018

sunway513 Mar 21, 2018

sabreshao Mar 22, 2018

helinwang commented Mar 21, 2018

reyoung left a comment


		list(APPEND EXTERNAL_LIBS "-L/opt/rocm/lib/ -lhip_hcc")

		set(HIP_HCC_FLAGS "${HIP_HCC_FLAGS} -fPIC -DPADDLE_WITH_HIP -std=c++14" )

Demostration of cmake refine for HIP support. #9165

Demostration of cmake refine for HIP support. #9165

Conversation

sabreshao commented Mar 16, 2018

CLAassistant commented Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyoung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang commented Mar 21, 2018

reyoung left a comment

Choose a reason for hiding this comment

CLAassistant commented Mar 16, 2018 •

edited

Loading