[Mesa-dev] [PATCH 12/13] clover: Add function for building a clover::module for non-TGSI targets v5

[Tom Stellard]

On Mon, May 28, 2012 at 10:52:35PM +0200, Francisco Jerez wrote:
> Tom Stellard <thomas.stellard at amd.com> writes:
> 
> > On Mon, May 28, 2012 at 10:03:27PM +0200, Francisco Jerez wrote:
> >> Tom Stellard <tstellar at gmail.com> writes:
> >> 
> >> > v2:
> >> >   -Separate IR type and LLVM triple
> >> >   -Do the OpenCL C->LLVM IR and linking steps for all PIPE_SHADER_IR
> >> >    types.
> >> >
> >> > v3:
> >> >   - Coding style fixes
> >> >   - Removed compatibility code for LLVM < 3.1
> >> >   - Split build_module_llvm() into three functions:
> >> >     compile(), link(), and build_module_llvm()
> >> >
> >> > v4:
> >> >   - Use struct pipe_compute_program
> >> >
> >> > v5:
> >> >   - Don't malloc memory for struct pipe_llvm_program
> >> > ---
> >> >  .../state_trackers/clover/core/compiler.hpp        |    2 +
> >> >  src/gallium/state_trackers/clover/core/program.cpp |    9 +-
> >> >  .../state_trackers/clover/llvm/invocation.cpp      |  165 ++++++++++++++++++--
> >> >  3 files changed, 162 insertions(+), 14 deletions(-)
> >[...]
> >> > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >> > index 89e21bf..30bad7c 100644
> >> > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >> > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >> >[...]
> >> > +      program.num_bytes = llvm_bitcode.size();
> >> > +      std::string data;
> >> > +      data.insert(0, (char*)(&program.num_bytes), 4);
> >> 
> >> What's the point of defining the header layout using a struct if you're
> >> pushing the header fields by hand into the object file?
> >>
> >
> > I was trying to follow the suggestion you gave in the last email as
> > close as possible:
> >
> >> It should be as simple as:
> >>
> >>|        header.num_bytes = llvm_bitcode.size();
> >>|        sec.data.insert(sec.data.end(), (char *)header,
> >>|                        (char *)header + sizeof(header));
> >>|        sec.data.insert(sec.data.end(), llvm_bitcode.begin(),
> >>|                        llvm_bitcode.end());
> >
> > However, I realized that if you serialize it this way, you end up
> > serializing garbage for the second member of struct pipe_llvm_program,
> > which is a char*.  This also defeats the purpose of having a struct.
> >
> I don't think it makes much sense to include that pointer in the struct,
> if the only purpose is to represent an LLVM program header.
> 

OK, I get it now.  I'll redefine the header as:

struct pipe_llvm_program {
   uint32_t num_bytes;
}

Then I'll just serialize the bytecode after the header.

-Tom

> > I've gone back and forth on this part several times, and I'm still not
> > sure what the correct solution is.  Do you have any other suggestions?
> >
> > -Tom
> >