[Beignet] [PATCH] add binary type support for compiled object and library.
Luo, Xionghu
xionghu.luo at intel.com
Mon Jun 16 00:44:10 PDT 2014
Two comments with ">>>>"
Luo Xionghu
Best Regards
-----Original Message-----
From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com]
Sent: Monday, June 16, 2014 11:34 AM
To: Luo, Xionghu
Cc: beignet at lists.freedesktop.org
Subject: Re: [Beignet] [PATCH] add binary type support for compiled object and library.
Some minor comments as below.
On Mon, Jun 16, 2014 at 05:27:20AM +0800, xionghu.luo at intel.com wrote:
> From: Luo <xionghu.luo at intel.com>
>
> save the llvm bitecode to program->binary: insert a bite in front of
> the
It should be bytecode or bitcode, right? :) here, it should be byte, right?
> bitcode stands for binary type(1 means COMPILED_OBJECT, 2 means
> LIBRARY); load the binary to module by ParseIR.
>
> create random directory to save compile header files.
> use strncpy and strncat to replace strcpy and strcat.
>
> Signed-off-by: Luo <xionghu.luo at intel.com>
>
> Conflicts:
> src/cl_api.c
> src/cl_gbe_loader.cpp
> src/cl_khr_icd.c
> src/cl_program.c
This is a rebase of a unmerged patch to current master. Not a cherry-pick in the tree, please don't leave the above conflicts in this type of patch's commit message.
> ---
> backend/src/backend/gen_program.cpp | 77 +++++++++++++++++++++++++++++++++----
> backend/src/backend/program.cpp | 1 +
> backend/src/backend/program.h | 8 +++-
> src/cl_api.c | 25 ++++++++++--
> src/cl_gbe_loader.cpp | 25 +++++++-----
> src/cl_gbe_loader.h | 10 +++--
> src/cl_program.c | 71 ++++++++++++++++++++++++++++++----
> src/cl_program.h | 1 +
> 8 files changed, 185 insertions(+), 33 deletions(-)
>
> diff --git a/backend/src/backend/gen_program.cpp
> b/backend/src/backend/gen_program.cpp
> index 300741e..2ef8307 100644
> --- a/backend/src/backend/gen_program.cpp
> +++ b/backend/src/backend/gen_program.cpp
> @@ -35,6 +35,12 @@
>
> #include "llvm/Linker.h"
> #include "llvm/Transforms/Utils/Cloning.h"
> +#include "llvm/Bitcode/ReaderWriter.h"
> +#include "llvm/Support/raw_ostream.h"
> +#include "llvm/ADT/StringRef.h"
> +#include "llvm/Support/MemoryBuffer.h"
> +#include "llvm/Support/SourceMgr.h"
> +#include "llvm/IRReader/IRReader.h"
>
> #include "backend/program.h"
> #include "backend/gen_program.h"
> @@ -55,6 +61,7 @@
> #include <iostream>
> #include <fstream>
> #include <mutex>
> +#include <unistd.h>
>
> namespace gbe {
>
> @@ -203,20 +210,75 @@ namespace gbe {
> return reinterpret_cast<gbe_program>(program);
> }
>
> - static size_t genProgramSerializeToBinary(gbe_program program, char
> **binary) {
> + static gbe_program genProgramNewFromLLVMBinary(uint32_t deviceID,
> +const char *binary, size_t size) { #ifdef GBE_COMPILER_AVAILABLE
> + using namespace gbe;
> + std::string binary_content;
> + //the first bit stands for binary_type.
Should be "the first byte" right?
> + binary_content.assign(binary+1, size-1);
> + llvm::StringRef llvm_bin_str(binary_content);
> + llvm::LLVMContext& c = llvm::getGlobalContext();
> + llvm::SMDiagnostic Err;
> + llvm::MemoryBuffer* memory_buffer = llvm::MemoryBuffer::getMemBuffer(llvm_bin_str, "llvm_bin_str");
> + llvm::Module* module = llvm::ParseIR(memory_buffer, Err, c);
> +
> + GenProgram *program = GBE_NEW(GenProgram, deviceID, module);
> +
> + //program->printStatus(0, std::cout);
> + return reinterpret_cast<gbe_program>(program);
> +#else
> + return NULL;
> +#endif
> + }
> +
> + static size_t genProgramSerializeToBinary(gbe_program program, char
> + **binary, int binary_type) {
> using namespace gbe;
> size_t sz;
> std::ostringstream oss;
> GenProgram *prog = (GenProgram*)program;
>
> - if ((sz = prog->serializeToBin(oss)) == 0) {
> - *binary = 0;
> + //0 means GEN binary, 1 means LLVM bitcode compiled object, 2 means LLVM bitcode library
> + if(binary_type == 0){
> + if ((sz = prog->serializeToBin(oss)) == 0) {
> + *binary = 0;
(*binary) is a pointer, it's better assign a NULL to it rather than a 0.
> + return 0;
> + }
> +
> + *binary = (char *)malloc(sizeof(char) * sz);
> + memcpy(*binary, oss.str().c_str(), sz*sizeof(char));
> + return sz;
> + }else{
> +#ifdef GBE_COMPILER_AVAILABLE
> + char llStr[] = "/tmp/XXXXXX.ll";
> + int llFd = mkstemps(llStr, 3);
> + close(llFd);
> + const std::string llName = std::string(llStr);
> + std::string errorInfo;
> +#if (LLVM_VERSION_MAJOR == 3) && (LLVM_VERSION_MINOR > 3)
> + auto mode = llvm::sys::fs::F_Binary; #else
> + auto mode = llvm::raw_fd_ostream::F_Binary; #endif
> + llvm::raw_fd_ostream OS(llName.c_str(), errorInfo, mode);
> + llvm::WriteBitcodeToFile((llvm::Module*)prog->module, OS);
> + OS.close();
> + FILE* pfile = fopen(llName.c_str(), "rb");
> + fseek(pfile, 0, SEEK_END);
> + int llsz = ftell(pfile);
> + rewind(pfile);
> + *binary = (char *)malloc(sizeof(char) * (llsz+1) );
> + int result = fread(*binary+1, 1, llsz, pfile);
> + if(result != llsz){
> + GBE_ASSERT(0);
> + }
> + *(*binary) = binary_type;
> + fclose(pfile);
> + remove(llName.c_str());
> + return llsz+1;
> +#else
> return 0;
> +#endif
> }
> -
> - *binary = (char *)malloc(sizeof(char) * sz);
> - memcpy(*binary, oss.str().c_str(), sz*sizeof(char));
> - return sz;
> }
>
> static gbe_program genProgramNewFromLLVM(uint32_t deviceID, @@
> -337,6 +399,7 @@ namespace gbe { void genSetupCallBacks(void) {
> gbe_program_new_from_binary = gbe::genProgramNewFromBinary;
> + gbe_program_new_from_llvm_binary =
> + gbe::genProgramNewFromLLVMBinary;
> gbe_program_serialize_to_binary = gbe::genProgramSerializeToBinary;
> gbe_program_new_from_llvm = gbe::genProgramNewFromLLVM;
> gbe_program_new_gen_program = gbe::genProgramNewGenProgram; diff
> --git a/backend/src/backend/program.cpp
> b/backend/src/backend/program.cpp index 45983fd..98e7ab7 100644
> --- a/backend/src/backend/program.cpp
> +++ b/backend/src/backend/program.cpp
> @@ -1158,6 +1158,7 @@ GBE_EXPORT_SYMBOL gbe_program_new_from_source_cb
> *gbe_program_new_from_source = GBE_EXPORT_SYMBOL
> gbe_program_compile_from_source_cb *gbe_program_compile_from_source =
> NULL; GBE_EXPORT_SYMBOL gbe_program_link_program_cb
> *gbe_program_link_program = NULL; GBE_EXPORT_SYMBOL
> gbe_program_new_from_binary_cb *gbe_program_new_from_binary = NULL;
> +GBE_EXPORT_SYMBOL gbe_program_new_from_llvm_binary_cb
> +*gbe_program_new_from_llvm_binary = NULL;
> GBE_EXPORT_SYMBOL gbe_program_serialize_to_binary_cb
> *gbe_program_serialize_to_binary = NULL; GBE_EXPORT_SYMBOL
> gbe_program_new_from_llvm_cb *gbe_program_new_from_llvm = NULL;
> GBE_EXPORT_SYMBOL gbe_program_new_gen_program_cb
> *gbe_program_new_gen_program = NULL; diff --git
> a/backend/src/backend/program.h b/backend/src/backend/program.h index
> 508fe64..c56b94a 100644
> --- a/backend/src/backend/program.h
> +++ b/backend/src/backend/program.h
> @@ -179,8 +179,12 @@ extern gbe_program_new_gen_program_cb
> *gbe_program_new_gen_program; typedef gbe_program
> (gbe_program_new_from_binary_cb)(uint32_t deviceID, const char
> *binary, size_t size); extern gbe_program_new_from_binary_cb
> *gbe_program_new_from_binary;
>
> -/*! Serialize a program to a bin */
> -typedef size_t (gbe_program_serialize_to_binary_cb)(gbe_program
> program, char **binary);
> +/*! Create a new program from the llvm bitcode*/ typedef gbe_program
> +(gbe_program_new_from_llvm_binary_cb)(uint32_t deviceID, const char
> +*binary, size_t size); extern gbe_program_new_from_llvm_binary_cb
> +*gbe_program_new_from_llvm_binary;
> +
> +/*! Serialize a program to a bin, 0 means executable, 1 means llvm
> +bitcode*/ typedef size_t
> +(gbe_program_serialize_to_binary_cb)(gbe_program program, char
> +**binary, int binary_type);
> extern gbe_program_serialize_to_binary_cb
> *gbe_program_serialize_to_binary;
>
> /*! Create a new program from the given LLVM file */ diff --git
> a/src/cl_api.c b/src/cl_api.c index 73447c4..4e069c4 100644
> --- a/src/cl_api.c
> +++ b/src/cl_api.c
> @@ -1065,8 +1065,16 @@ clGetProgramInfo(cl_program program,
> FILL_GETINFO_RET (char, (strlen(program->source) + 1),
> program->source, CL_SUCCESS);
> } else if (param_name == CL_PROGRAM_BINARY_SIZES) {
> - if (program->binary == NULL) {
> - program->binary_sz = compiler_program_serialize_to_binary(program->opaque, &program->binary);
> + if (program->binary == NULL){
> + if( program->binary_type == CL_PROGRAM_BINARY_TYPE_EXECUTABLE) {
> + program->binary_sz = compiler_program_serialize_to_binary(program->opaque, &program->binary, 0);
> + }else if( program->binary_type == CL_PROGRAM_BINARY_TYPE_COMPILED_OBJECT) {
> + program->binary_sz = compiler_program_serialize_to_binary(program->opaque, &program->binary, 1);
> + }else if( program->binary_type == CL_PROGRAM_BINARY_TYPE_LIBRARY) {
> + program->binary_sz = compiler_program_serialize_to_binary(program->opaque, &program->binary, 2);
> + }else{
> + assert(0);
> + }
> }
>
> if (program->binary == NULL || program->binary_sz == 0) {
> @@ -1082,7 +1090,15 @@ clGetProgramInfo(cl_program program,
> /* param_value points to an array of n
> pointers allocated by the caller */
> if (program->binary == NULL) {
> - program->binary_sz = compiler_program_serialize_to_binary(program->opaque, &program->binary);
> + if( program->binary_type == CL_PROGRAM_BINARY_TYPE_EXECUTABLE) {
> + program->binary_sz = compiler_program_serialize_to_binary(program->opaque, &program->binary, 0);
> + }else if( program->binary_type == CL_PROGRAM_BINARY_TYPE_COMPILED_OBJECT) {
> + program->binary_sz = compiler_program_serialize_to_binary(program->opaque, &program->binary, 1);
> + }else if( program->binary_type == CL_PROGRAM_BINARY_TYPE_LIBRARY) {
> + program->binary_sz = compiler_program_serialize_to_binary(program->opaque, &program->binary, 2);
> + }else{
> + assert(0);
> + }
> }
>
> if (program->binary == NULL || program->binary_sz == 0) {
> @@ -1134,6 +1150,9 @@ clGetProgramBuildInfo(cl_program program,
> FILL_GETINFO_RET (char, program->build_log_sz + 1, program->build_log, CL_SUCCESS);
> if (param_value_size_ret)
> *param_value_size_ret = program->build_log_sz + 1;
> + }else if (param_name == CL_PROGRAM_BINARY_TYPE){
> +
> + FILL_GETINFO_RET (cl_uint, 1, &program->binary_type, CL_SUCCESS);
> } else {
> return CL_INVALID_VALUE;
> }
> diff --git a/src/cl_gbe_loader.cpp b/src/cl_gbe_loader.cpp index
> 470299b..debef4b 100644
> --- a/src/cl_gbe_loader.cpp
> +++ b/src/cl_gbe_loader.cpp
> @@ -31,6 +31,7 @@ gbe_program_compile_from_source_cb
> *compiler_program_compile_from_source = NULL;
> gbe_program_new_gen_program_cb *compiler_program_new_gen_program =
> NULL; gbe_program_link_program_cb *compiler_program_link_program =
> NULL; gbe_program_build_from_llvm_cb
> *compiler_program_build_from_llvm = NULL;
> +gbe_program_new_from_llvm_binary_cb
> +*compiler_program_new_from_llvm_binary = NULL;
>
> //function pointer from libgbeinterp.so
> gbe_program_new_from_binary_cb *interp_program_new_from_binary = NULL;
> @@ -256,6 +257,18 @@ struct GbeLoaderInitializer
> if (compiler_program_new_from_source == NULL)
> return;
>
> + compiler_program_serialize_to_binary = *(gbe_program_serialize_to_binary_cb **)dlsym(dlhCompiler, "gbe_program_serialize_to_binary");
> + if (compiler_program_serialize_to_binary == NULL)
> + return;
> +
> + compiler_program_new_from_llvm = *(gbe_program_new_from_llvm_cb **)dlsym(dlhCompiler, "gbe_program_new_from_llvm");
> + if (compiler_program_new_from_llvm == NULL)
> + return;
> +
> + compiler_set_image_base_index = *(gbe_set_image_base_index_cb **)dlsym(dlhCompiler, "gbe_set_image_base_index");
> + if (compiler_set_image_base_index == NULL)
> + return;
> +
> compiler_program_compile_from_source = *(gbe_program_compile_from_source_cb **)dlsym(dlhCompiler, "gbe_program_compile_from_source");
> if (compiler_program_compile_from_source == NULL)
> return;
> @@ -272,16 +285,8 @@ struct GbeLoaderInitializer
> if (compiler_program_build_from_llvm == NULL)
> return;
>
> - compiler_program_serialize_to_binary = *(gbe_program_serialize_to_binary_cb **)dlsym(dlhCompiler, "gbe_program_serialize_to_binary");
> - if (compiler_program_serialize_to_binary == NULL)
> - return;
> -
> - compiler_program_new_from_llvm = *(gbe_program_new_from_llvm_cb **)dlsym(dlhCompiler, "gbe_program_new_from_llvm");
> - if (compiler_program_new_from_llvm == NULL)
> - return;
> -
> - compiler_set_image_base_index = *(gbe_set_image_base_index_cb **)dlsym(dlhCompiler, "gbe_set_image_base_index");
> - if (compiler_set_image_base_index == NULL)
Actually, the above two changes have no meaningless. And I know it was generated due to the rebase. When we do rebase, it's better to avoid to generate this type of changes.
>>>> adjusted the sequence to match the definition.
> + compiler_program_new_from_llvm_binary = *(gbe_program_new_from_llvm_binary_cb **)dlsym(dlhCompiler, "gbe_program_new_from_llvm_binary");
> + if (compiler_program_new_from_llvm_binary == NULL)
> return;
>
> compilerLoaded = true;
> diff --git a/src/cl_gbe_loader.h b/src/cl_gbe_loader.h index
> bd6afe6..9759bac 100644
> --- a/src/cl_gbe_loader.h
> +++ b/src/cl_gbe_loader.h
> @@ -28,6 +28,12 @@ extern gbe_program_new_from_source_cb
> *compiler_program_new_from_source;
> extern gbe_program_serialize_to_binary_cb
> *compiler_program_serialize_to_binary;
> extern gbe_program_new_from_llvm_cb *compiler_program_new_from_llvm;
> extern gbe_set_image_base_index_cb *compiler_set_image_base_index;
> +extern gbe_program_compile_from_source_cb
> +*compiler_program_compile_from_source;
> +extern gbe_program_new_gen_program_cb
> +*compiler_program_new_gen_program;
> +extern gbe_program_link_program_cb *compiler_program_link_program;
> +extern gbe_program_build_from_llvm_cb
> +*compiler_program_build_from_llvm;
> +extern gbe_program_new_from_llvm_binary_cb
> +*compiler_program_new_from_llvm_binary;
> +
> extern gbe_program_new_from_binary_cb
> *interp_program_new_from_binary; extern
> gbe_program_get_global_constant_size_cb
> *interp_program_get_global_constant_size;
> extern gbe_program_get_global_constant_data_cb
> *interp_program_get_global_constant_data;
> @@ -63,10 +69,6 @@ extern gbe_get_printf_sizeof_size_cb*
> interp_get_printf_sizeof_size; extern gbe_release_printf_info_cb*
> interp_release_printf_info; extern gbe_output_printf_cb*
> interp_output_printf; //extern gbe_set_image_base_index_cb
> *gbe_set_image_base_index_interp; -extern
> gbe_program_compile_from_source_cb
> *compiler_program_compile_from_source;
> -extern gbe_program_new_gen_program_cb
> *compiler_program_new_gen_program;
> -extern gbe_program_link_program_cb *compiler_program_link_program;
> -extern gbe_program_build_from_llvm_cb
> *compiler_program_build_from_llvm;
The same comment as the previous one.
> extern gbe_kernel_get_arg_info_cb *interp_kernel_get_arg_info;
>
> int CompilerSupported();
> diff --git a/src/cl_program.c b/src/cl_program.c index
> 240453c..2f779f1 100644
> --- a/src/cl_program.c
> +++ b/src/cl_program.c
> @@ -156,6 +156,30 @@ error:
> return err;
> }
>
> +inline cl_bool isBitcodeWrapper(const unsigned char *BufPtr, const
> +unsigned char *BufEnd) {
> + // See if you can find the hidden message in the magic bytes :-).
> + // // (Hint: it's a little-endian encoding.)
please remove the second //
> + return BufPtr != BufEnd &&
> + BufPtr[0] == 0xDE &&
> + BufPtr[1] == 0xC0 &&
> + BufPtr[2] == 0x17 &&
> + BufPtr[3] == 0x0B;
> +}
> +
> +inline cl_bool isRawBitcode(const unsigned char *BufPtr, const
> +unsigned char *BufEnd) {
> + // These bytes sort of have a hidden message, but it's not in
> + // // little-endian this time, and it's a little redundant.
please remove the second //. Why do you have to design two different endianness here.
why not just always use little-endian? And just read the u32 and compare the u32 data
to the magic const directly?
>>>> isBitcodeWrapper and isRawBitcode functions are copied from llvm source code.
The other part LGTM.
More information about the Beignet
mailing list