[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: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/core/program.cpp b/src/gallium/state_trackers/clover/core/program.cpp
> > index 06ac2af..8e34696 100644
> > --- a/src/gallium/state_trackers/clover/core/program.cpp
> > +++ b/src/gallium/state_trackers/clover/core/program.cpp
> > @@ -47,9 +47,14 @@ _cl_program::build(const std::vector<clover::device *> &devs) {
> >  
> >     for (auto dev : devs) {
> >        try {
> > -         auto module = (dev->ir_target() == "tgsi" ?
> > +         // XXX: We need to check the input source to determine which
> > +         //      compile_program() call to use.  If the input is TGSI we
> > +         //      should use compile_program_tgsi, otherwise we should use
> > +         //      compile_program_llvm
> 
> I don't think we need to do that, we'll get rid of the support code for
> compiling TGSI kernels once the CL->TGSI translation pass is ready.
> 
> > +         auto module = (dev->ir_format() == PIPE_SHADER_IR_TGSI ?
> >                          compile_program_tgsi(__source) :
> > -                        compile_program_llvm(__source, dev->ir_target()));
> > +                        compile_program_llvm(__source, dev->ir_format(),
> > +			                     dev->ir_target()));
> >           __binaries.insert({ dev, module });
> >  
> >        } catch (build_error &e) {
> > 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'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

> > +      data.insert(data.end(), llvm_bitcode.begin(),
> > +                                  llvm_bitcode.end());
> > +      m.syms.push_back(module::symbol(kernel_name, 0, 0, args ));
> > +      m.secs.push_back(module::section(0, module::section::text, data.size(),
> > +                                       data));
> 
> This should pass 'llvm_bitcode.size()' instead of 'data.size()'.
> 'section::size' is supposed to be the virtual section size in memory,
> excluding headers.
> 
> > +
> > +      return m;
> > +   }
> > +} // End anonymous namespace
> > +
> >[...]