[Beignet] [PATCH] BGE: add param to switch the behavior of math func

[Guo, Yejun]

Thanks Zhigang, I looked a little deeper into make -j4 issue and found the reason is that update_blob_ocl_header.py is designed for single process. To simplify cmake script, agree to adjust it.

Glad to know the command to check trailing whitespace, something was missed by my manually check, :(

Thanks
Yejun

-----Original Message-----
From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com] 
Sent: Thursday, February 13, 2014 3:11 PM
To: Guo, Yejun
Cc: beignet at lists.freedesktop.org
Subject: Re: [Beignet] [PATCH] BGE: add param to switch the behavior of math func

One typo in the subject, should use GBE rather than BGE.

Some comments as below:

On Thu, Feb 13, 2014 at 06:14:37AM +0800, Guo Yejun wrote:
> Add OCL_STRICT_CONFORMANCE to switch the behavior of math func,
> The funcs will be high precision with perf drops if it is 1,
> Fast path with good enough precision will be selected if it is 0.
> 
> This change is to add the code basis, with 'sin' implmented as
> an example, other math functions support will be added later.
> 
> Signed-off-by: Guo Yejun <yejun.guo at intel.com>
> ---
>  backend/CMakeLists.txt          |    3 ++-
>  backend/src/CMakeLists.txt      |   18 +++++++++++++++++-
>  backend/src/GBEConfig.h.in      |    1 +
>  backend/src/backend/program.cpp |   12 +++++++++++-
>  backend/src/ocl_stdlib.tmpl.h   |   27 +++++++++++++++++++++++++++
>  utests/setenv.sh.in             |    1 +
>  6 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/backend/CMakeLists.txt b/backend/CMakeLists.txt
> index dd55a4a..08122da 100644
> --- a/backend/CMakeLists.txt
> +++ b/backend/CMakeLists.txt
> @@ -98,8 +98,9 @@ include_directories (${CMAKE_CURRENT_BINARY_DIR})
>  ##############################################################
>  add_subdirectory (src)
>  set(LOCAL_PCH_OBJECT_DIR ${LOCAL_PCH_OBJECT_DIR} PARENT_SCOPE)
> +set(LOCAL_PCH_STRICT_OBJECT_DIR ${LOCAL_PCH_STRICT_OBJECT_DIR} PARENT_SCOPE)
>  set(LOCAL_PCM_OBJECT_DIR ${LOCAL_PCM_OBJECT_DIR} PARENT_SCOPE)
>  set (GBE_BIN_GENERATER
> -     OCL_PCM_PATH=${LOCAL_PCM_OBJECT_DIR} OCL_PCH_PATH=${LOCAL_PCH_OBJECT_DIR} ${CMAKE_CURRENT_BINARY_DIR}/src/gbe_bin_generater
> +     OCL_PCM_PATH=${LOCAL_PCM_OBJECT_DIR} OCL_PCH_PATH=${LOCAL_PCH_OBJECT_DIR} OCL_PCH_STRICT_PATH=${LOCAL_PCH_STRICT_OBJECT_DIR} ${CMAKE_CURRENT_BINARY_DIR}/src/gbe_bin_generater
>       PARENT_SCOPE)
>  
> diff --git a/backend/src/CMakeLists.txt b/backend/src/CMakeLists.txt
> index 33494a0..775f19b 100644
> --- a/backend/src/CMakeLists.txt
> +++ b/backend/src/CMakeLists.txt
> @@ -43,6 +43,8 @@ add_custom_command(
>  
>  set (pch_object ${ocl_blob_file}.pch)
>  set (local_pch_object ${ocl_blob_file}.local.pch)
> +set (pch_strict_object ${ocl_blob_file}.strict.pch)
> +set (local_pch_strict_object ${ocl_blob_file}.strict.local.pch)
>  # generate pch object
>  if (LLVM_VERSION_NODOT VERSION_GREATER 32)
>      set (clang_cmd -cc1 -x cl -triple spir -ffp-contract=off)
> @@ -66,6 +68,17 @@ add_custom_command(
>  add_custom_target(pch_object
>                    DEPENDS ${pch_object})
>  
> +add_custom_command(
> +     OUTPUT ${pch_strict_object}
> +     COMMAND rm -f ${pch_strict_object}
> +     COMMAND clang ${clang_cmd} -DOCL_STRICT_CONFORMANCE=1 --relocatable-pch -emit-pch -isysroot ${CMAKE_CURRENT_BINARY_DIR} ${ocl_blob_file} -o ${pch_strict_object}
> +     COMMAND clang ${clang_cmd} -DOCL_STRICT_CONFORMANCE=1 -emit-pch ${ocl_blob_file} -o ${local_pch_strict_object}
> +     DEPENDS ${ocl_blob_file}
> +     )
> +
> +add_custom_target(pch_strict_object
> +                  DEPENDS ${pch_strict_object})
> +

Should not set pch_strict_object as another target, you just need to add the
commands using to generate pch_strict_object and local_pch_strict_object to
the place after the normal pch_object/local_pch_object generation.

As both objects depend on the same file set, this can simplfy the cmake script
and could avoid the -j4 issue. You can try to use make -j4 with you current patch.
It will fail.

>  macro(ll_add_library ll_lib ll_sources)
>    foreach (ll ${${ll_sources}})
>    add_custom_command(
> @@ -176,7 +189,7 @@ set (pcm_lib "beignet.bc")
>  set (pcm_sources ocl_barrier.ll ocl_memset.ll ocl_memcpy.ll)
>  ll_add_library (${pcm_lib} pcm_sources)
>  
> -ADD_DEPENDENCIES (gbe pch_object ${pcm_lib})
> +ADD_DEPENDENCIES (gbe pch_object pch_strict_object ${pcm_lib})
Just delete the pch_strict_object here.
>  target_link_libraries(
>                        gbe
>                        ${DRM_INTEL_LIBRARY}
> @@ -196,13 +209,16 @@ TARGET_LINK_LIBRARIES(gbe_bin_generater gbe)
>  #install (FILES backend/program.h DESTINATION include/gen)
>  install (FILES ${ocl_blob_file} DESTINATION ${LIB_INSTALL_DIR}/beignet)
>  install (FILES ${pch_object} DESTINATION ${LIB_INSTALL_DIR}/beignet)
> +install (FILES ${pch_strict_object} DESTINATION ${LIB_INSTALL_DIR}/beignet)
>  install (FILES ${CMAKE_CURRENT_BINARY_DIR}/${pcm_lib} DESTINATION ${LIB_INSTALL_DIR}/beignet)
>  # 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_path}/ocl_stdlib.h.pch" PARENT_SCOPE)
> +set (LOCAL_PCH_STRICT_OBJECT_DIR "${local_pch_object}:${beignet_install_path}/ocl_stdlib.h.strict.pch" PARENT_SCOPE)
                                       ~~~~~~~~~~~~~~~~ should use local_pch_strict_object.


And you may need to use git show --check and fix all the format issue before
submit the patch to the list. Take this patch as an example, git show --check
found the following warnings:

backend/src/ocl_stdlib.tmpl.h:4467: trailing whitespace.
+/// It is required by OpenCL that built-in math functions (without native_/half_)
backend/src/ocl_stdlib.tmpl.h:4468: trailing whitespace.
+/// have high precision, but GPU hardware is designed to be good enough precision,
backend/src/ocl_stdlib.tmpl.h:4474: trailing whitespace.
+/// of the math functions.