Two comments with ">>>>" Luo Xionghu Best Regards
-----Original Message----- From: Zhigang Gong [mailto:[email protected]] Sent: Monday, June 16, 2014 11:34 AM To: Luo, Xionghu Cc: [email protected] 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, [email protected] wrote: > From: Luo <[email protected]> > > 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 <[email protected]> > > 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. _______________________________________________ Beignet mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/beignet
