[Beignet] [PATCH 01/18] Add the ocl header file and dir for bitcode library.

[Zhigang Gong]

Hi Junyan,

Some comments for this patch are embedded below, please check them out.

On Tue, Aug 12, 2014 at 03:31:08PM +0800, junyan.he at inbox.com wrote:
> From: Junyan He <junyan.he at linux.intel.com>
> 
> The first step for seperating the defination from the ocl
> huge header file. The ocl.h will include all the header
> files which will be used. All the bit code will be compiled
> as a bitcode lib named beignet.bc.
> 
> Signed-off-by: Junyan He <junyan.he at linux.intel.com>
> ---
>  backend/CMakeLists.txt                     |   3 +
>  backend/src/CMakeLists.txt                 |  20 ++++++
>  backend/src/GBEConfig.h.in                 |   2 +
>  backend/src/libocl/Makefile.in             |  39 +++++++++++
Why do you have to use manually wrote Makefile here? It's better to use
Cmake to keep consistent with the whole project.

>  backend/src/libocl/include/ocl.h           |   7 ++
>  backend/src/libocl/include/ocl_defines.inh |  23 +++++++
Just as we discussed, .inh looks not a good suffix name for a
header file. Maybe change to .tmpl.h is better.


>  backend/src/libocl/include/ocl_types.h     | 104 +++++++++++++++++++++++++++++
>  backend/src/libocl/lib/beignet.bc          |   0
>  9 files changed, 199 insertions(+), 1 deletion(-)
>  create mode 100644 backend/src/libocl/Makefile.in
>  create mode 100644 backend/src/libocl/include/ocl.h
>  create mode 100644 backend/src/libocl/include/ocl_defines.inh
>  create mode 100644 backend/src/libocl/include/ocl_types.h
>  create mode 100644 backend/src/libocl/lib/beignet.bc
beignet.bc should be a generated binary file, it should not be commited
to beignet repo. Right? If the lib directory is to put generated bit code, we
should not create it at the source code tree. Just create it at build time and
put the generated file to the build directory.

> 
> diff --git a/backend/CMakeLists.txt b/backend/CMakeLists.txt
> index ca674cf..8734c2a 100644
> --- a/backend/CMakeLists.txt
> +++ b/backend/CMakeLists.txt
> @@ -101,6 +101,9 @@ include_directories (${CMAKE_CURRENT_BINARY_DIR})
>  add_subdirectory (src)
>  set(LOCAL_PCH_OBJECT_DIR ${LOCAL_PCH_OBJECT_DIR} PARENT_SCOPE)
>  set(LOCAL_PCM_OBJECT_DIR ${LOCAL_PCM_OBJECT_DIR} PARENT_SCOPE)
> +set(LOCAL_OCL_BITCODE_BIN "${LOCAL_OCL_BITCODE_BIN}" PARENT_SCOPE)
> +set(LOCAL_OCL_HEADER_DIR "${LOCAL_OCL_HEADER_DIR}" PARENT_SCOPE)
> +
>  set(LOCAL_GBE_OBJECT_DIR ${LOCAL_GBE_OBJECT_DIR} PARENT_SCOPE)
>  set(LOCAL_INTERP_OBJECT_DIR ${LOCAL_INTERP_OBJECT_DIR} PARENT_SCOPE)
>  
> diff --git a/backend/src/CMakeLists.txt b/backend/src/CMakeLists.txt
> index 85b5e21..63eb0f0 100644
> --- a/backend/src/CMakeLists.txt
> +++ b/backend/src/CMakeLists.txt
> @@ -1,3 +1,6 @@
> +set (OCL_BITCODE_BIN "${BEIGNET_INSTALL_DIR}beignet.bc")
> +set (OCL_BITCODE_DIR "${BEIGNET_INSTALL_DIR}")
If it is the same as BEIGNET_INSTALL_DIR, it is not necessary to create a new macro here.
Just use the common value directly.

> +set (OCL_HEADER_DIR "${CMAKE_INSTALL_PREFIX}/include/CL/ocl/")
This is not good. The include/CL directly should contain standard OCL header files,
and in the future, we may not embedd those header files into beignet and use a pre-installed
OCL header package instead. So please don't try to install beignet specific files into
this place. There is only one exception which is for private extension header file which
is not part of the standard, but we want the user use it as a runtime OCL API, and you
can check out the cl_intel.h which is such an example. But the OCL_HEADER
is just a header for OCL kernel use implicitly, please keep it stay with beignet's library
and don't put it into public header directory.

>  set (ocl_vector_spec_file ${GBE_SOURCE_DIR}/src/builtin_vector_proto.def)
>  set (ocl_vector_file ${GBE_SOURCE_DIR}/src/ocl_vector.h)
>  set (ocl_as_file ${GBE_SOURCE_DIR}/src/ocl_as.h)
> @@ -60,6 +63,14 @@ add_custom_command(
>       DEPENDS ${ocl_blob_file}
>       )
>  
> +add_custom_command (
> +    OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/libocl/lib/beignet.bc
Comment
> +    COMMAND cd ${CMAKE_CURRENT_SOURCE_DIR}/libocl && make
> +    )
> +
> +add_custom_target(beignet_bitcode ALL
> +    DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/libocl/lib/beignet.bc
> +    )
If we put beignet.bc into build directory, you should fix the above directory path as well.

>  add_custom_target(pch_object
>                    DEPENDS ${pch_object})
>  
> @@ -201,6 +212,8 @@ target_link_libraries(
>  
>  add_library(gbeinterp SHARED gbe_bin_interpreter.cpp)
>  
> +add_dependencies(gbe beignet_bitcode)
> +
>  if (LLVM_VERSION_NODOT VERSION_EQUAL 34)
>    find_library(TERMINFO NAMES tinfo ncurses)
>    if (${TERMINFO} STREQUAL TERMINFO-NOTFOUND)
> @@ -221,6 +234,9 @@ install (TARGETS gbeinterp LIBRARY DESTINATION ${BEIGNET_INSTALL_DIR})
>  install (FILES ${ocl_blob_file} DESTINATION ${BEIGNET_INSTALL_DIR})
>  install (FILES ${pch_object} DESTINATION ${BEIGNET_INSTALL_DIR})
>  install (FILES ${CMAKE_CURRENT_BINARY_DIR}/${pcm_lib} DESTINATION ${BEIGNET_INSTALL_DIR})
> +install (FILES ${CMAKE_CURRENT_SOURCE_DIR}/libocl/lib/beignet.bc DESTINATION ${OCL_BITCODE_DIR})
> +install (DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/libocl/include/ DESTINATION ${OCL_HEADER_DIR} PATTERN *.h)
> +
>  # When build beignet itself, we need to export the local precompiled header file and precompiled module
>  # file to libcl and utests.
>  set (LOCAL_PCH_OBJECT_DIR "${local_pch_object}:${BEIGNET_INSTALL_DIR}/ocl_stdlib.h.pch" PARENT_SCOPE)
> @@ -236,3 +252,7 @@ configure_file (
>    "GBEConfig.h.in"
>    "GBEConfig.h"
>  )
> +configure_file (
> +    "libocl/Makefile.in"
> +    "libocl/Makefile"
> +    )
> diff --git a/backend/src/GBEConfig.h.in b/backend/src/GBEConfig.h.in
> index f5c69c6..6f9d57f 100644
> --- a/backend/src/GBEConfig.h.in
> +++ b/backend/src/GBEConfig.h.in
> @@ -5,3 +5,5 @@
>  #define PCM_OBJECT_DIR "@PCM_OBJECT_DIR@"
>  #define GBE_OBJECT_DIR "@GBE_OBJECT_DIR@"
>  #define INTERP_OBJECT_DIR "@INTERP_OBJECT_DIR@"
> +#define OCL_BITCODE_BIN "@OCL_BITCODE_BIN@"
> +#define OCL_HEADER_DIR "@OCL_HEADER_DIR@"
> diff --git a/backend/src/libocl/Makefile.in b/backend/src/libocl/Makefile.in

Please change to use cmake to implement the same logic if possible.


Thanks,
Zhigang Gong