[Beignet] [PATCH] add binary type support for compiled object and library.
Zhigang Gong
zhigang.gong at linux.intel.com
Mon Jun 16 00:14:21 PDT 2014
On Mon, Jun 16, 2014 at 07:44:10AM +0000, Luo, Xionghu wrote:
> 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.
Right.
>
> > + 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.
OK, then you may need to put a comment there to indicate the following two functions
are copied from LLVM.
>
> The other part LGTM.
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
More information about the Beignet
mailing list