[Mesa-dev] [PATCH 1/3] clover: separate compile and link stages

EdB edb+mesa at sigluy.net
Sun Jul 5 11:41:58 PDT 2015


On Sunday 05 July 2015 18:15:33 Francisco Jerez wrote:
> Hi EdB, a bunch of comments inline,

Hello

> 
> EdB <edb+mesa at sigluy.net> writes:
> > ---
> > 
> >  src/gallium/state_trackers/clover/api/program.cpp  |   6 +-
> >  .../state_trackers/clover/core/compiler.hpp        |   7 +-
> >  src/gallium/state_trackers/clover/core/error.hpp   |  21 ++
> >  src/gallium/state_trackers/clover/core/program.cpp |  93 ++++++-
> >  src/gallium/state_trackers/clover/core/program.hpp |  10 +-
> >  .../state_trackers/clover/llvm/invocation.cpp      | 281
> >  +++++++++++++++------ 6 files changed, 323 insertions(+), 95
> >  deletions(-)
> > 
> > diff --git a/src/gallium/state_trackers/clover/api/program.cpp
> > b/src/gallium/state_trackers/clover/api/program.cpp index
> > e9b1f38..2441d81 100644
> > --- a/src/gallium/state_trackers/clover/api/program.cpp
> > +++ b/src/gallium/state_trackers/clover/api/program.cpp
> > @@ -184,10 +184,6 @@ clBuildProgram(cl_program d_prog, cl_uint num_devs,
> > 
> >     prog.build(devs, opts);
> 
> I don't think there's any reason to keep the program::build method
> around anymore, it's only going to be called from this entry point so
> you could as well make the two function calls to ::compile() and
> 
> ::link() directly from here.

ok

> ::
> >     return CL_SUCCESS;
> >  
> >  } catch (error &e) {
> > 
> > -   if (e.get() == CL_INVALID_COMPILER_OPTIONS)
> > -      return CL_INVALID_BUILD_OPTIONS;
> > -   if (e.get() == CL_COMPILE_PROGRAM_FAILURE)
> > -      return CL_BUILD_PROGRAM_FAILURE;
> > 
> >     return e.get();
> >  
> >  }
> > 
> > @@ -224,7 +220,7 @@ clCompileProgram(cl_program d_prog, cl_uint num_devs,
> > 
> >        range(header_names, num_headers),
> >        objs<allow_empty_tag>(d_header_progs, num_headers));
> > 
> > -   prog.build(devs, opts, headers);
> > +   prog.compile(devs, opts, headers);
> > 
> >     return CL_SUCCESS;
> >  
> >  } catch (error &e) {
> > 
> > diff --git a/src/gallium/state_trackers/clover/core/compiler.hpp
> > b/src/gallium/state_trackers/clover/core/compiler.hpp index
> > c68aa39..31fb6ee 100644
> > --- a/src/gallium/state_trackers/clover/core/compiler.hpp
> > +++ b/src/gallium/state_trackers/clover/core/compiler.hpp
> > @@ -32,11 +32,16 @@ namespace clover {
> > 
> >     module compile_program_llvm(const std::string &source,
> >     
> >                                 const header_map &headers,
> > 
> > -                               pipe_shader_ir ir,
> > 
> >                                 const std::string &target,
> >                                 const std::string &opts,
> >                                 std::string &r_log);
> > 
> > +   module link_program_llvm(const std::vector<module> &modules,
> > +                            enum pipe_shader_ir ir,
> > +                            const std::string &target,
> > +                            const std::string &opts,
> > +                            std::string &r_log);
> > +
> > 
> >     module compile_program_tgsi(const std::string &source);
> >  
> >  }
> > 
> > diff --git a/src/gallium/state_trackers/clover/core/error.hpp
> > b/src/gallium/state_trackers/clover/core/error.hpp index 780b973..3c1bf90
> > 100644
> > --- a/src/gallium/state_trackers/clover/core/error.hpp
> > +++ b/src/gallium/state_trackers/clover/core/error.hpp
> > @@ -68,10 +68,31 @@ namespace clover {
> > 
> >     class build_error : public error {
> >     
> >     public:
> >        build_error(const std::string &what = "") :
> > +         error(CL_BUILD_PROGRAM_FAILURE, what) {
> > +      }
> > +   };
> > +
> 
> This exception class now seems redundant -- With program::build() gone
> build is no longer a thing.

It's still needed by tgsi.
I plan to rework this part later to make it consistent with the way it's 
handle in llvm/invocation but first off I wanted to be done with clLink :/

> 
> > +   class compile_error : public error {
> > +   public:
> > 
> > +      compile_error(const std::string &what = "") :
> >           error(CL_COMPILE_PROGRAM_FAILURE, what) {
> >        
> >        }
> >     
> >     };
> > 
> > +   class link_error : public error {
> > +   public:
> > +      link_error(const std::string &what = "") :
> > +         error(CL_LINK_PROGRAM_FAILURE, what) {
> > +      }
> > +   };
> > +
> > +   class link_option_error : public error {
> > +   public:
> > +      link_option_error(const std::string &what = "") :
> > +         error(CL_INVALID_LINKER_OPTIONS , what) {
> > +      }
> > +   };
> > +
> 
> I don't think you really need to special-case link_option_error against
> the less specific clover::error class?

clLinkProgram should not create a program if it failed to parse the given 
options, I will use this class to handle this case. Other case should create 
the said program.
That said, it could also have been created in later patch.

> 
> >     template<typename O>
> >     class invalid_object_error;
> > 
> > diff --git a/src/gallium/state_trackers/clover/core/program.cpp
> > b/src/gallium/state_trackers/clover/core/program.cpp index
> > 0d6cc40..21faf4e 100644
> > --- a/src/gallium/state_trackers/clover/core/program.cpp
> > +++ b/src/gallium/state_trackers/clover/core/program.cpp
> > @@ -40,15 +40,37 @@ program::program(clover::context &ctx,
> > 
> >  }
> >  
> >  void
> > 
> > -program::build(const ref_vector<device> &devs, const char *opts,
> > -               const header_map &headers) {
> > +program::build(const ref_vector<device> &devs, const char *opts) {
> > +
> 
> Unnecessary whitespace.

ok

> 
> > +   if (has_source) {
> > +      try {
> > +         compile(devs, opts, {});
> > +         if (!link(devs, opts, {*this}, true))
> > +            throw error(CL_BUILD_PROGRAM_FAILURE);
> > +      } catch (error &e) {
> > +         switch (e.get()) {
> > +            case CL_INVALID_COMPILER_OPTIONS:
> > +            case CL_INVALID_LINKER_OPTIONS:
> > +               e = error(CL_INVALID_BUILD_OPTIONS);
> > +               break;
> > +            case CL_COMPILE_PROGRAM_FAILURE:
> > +            case CL_LINK_PROGRAM_FAILURE:
> > +               e = error(CL_BUILD_PROGRAM_FAILURE);
> > +               break;
> 
> Ugh...  This is mutating the caught exception through the l-value
> reference and rethrowing.  Just saying 'throw error(...)' in each case
> statement would be cleaner.  Doesn't matter anyway since this function
> is going away.

ok, I should have keep my first impression.

> 
> > +         }
> > +         throw;
> > +      }
> > +   }
> > +}
> > +
> > +void
> > +program::compile(const ref_vector<device> &devs, const char *opts,
> > +                 const header_map &headers) {
> > 
> >     if (has_source) {
> >     
> >        _devices = devs;
> >        
> >        for (auto &dev : devs) {
> > 
> > -         _binaries.erase(&dev);
> > -         _logs.erase(&dev);
> > -         _opts.erase(&dev);
> > +         clean(&dev);
> > 
> >           _opts.insert({ &dev, opts });
> > 
> > @@ -58,12 +80,11 @@ program::build(const ref_vector<device> &devs, const
> > char *opts,> 
> >              auto module = (dev.ir_format() == PIPE_SHADER_IR_TGSI ?
> >              
> >                             compile_program_tgsi(_source) :
> >                             compile_program_llvm(_source, headers,
> > 
> > -                                                dev.ir_format(),
> > 
> >                                                  dev.ir_target(),
> >                                                  build_opts(dev),
> >                                                  log));
> >              
> >              _binaries.insert({ &dev, module });
> >              _logs.insert({ &dev, log });
> > 
> > -         } catch (const build_error &) {
> > +         } catch (const compile_error &) {
> > 
> >              _logs.insert({ &dev, log });
> >              throw;
> >           
> >           }
> > 
> > @@ -71,6 +92,64 @@ program::build(const ref_vector<device> &devs, const
> > char *opts,> 
> >     }
> >  
> >  }
> > 
> > +bool
> > +program::link(const ref_vector<device> &devs, const char *opts,
> > +              const ref_vector<program> &progs, bool keep_log) {
> 
> I don't think that the keep_log option is very useful, link could just
> preserve the log in all cases -- That would work for clLinkProgram
> because there's no log in the first place, and for clBuildProgram too
> because the log has to be preserved.  (And boolean flags that alter the
> semantics of a function are a rather dubious practice in general).

indeed

> 
> > +
> 
> Unnecessary whitespace.

ok

> 
> > +   bool r = true;
> > +
> > +   _devices = devs;
> > +
> > +   for (auto &dev : devs) {
> > +      std::vector<module> mods;
> > +      mods.reserve(progs.size());
> > +      for (auto &prog : progs)
> > +         mods.push_back(prog.binary(dev));
> > +
> 
>       const std::vector<module> mods = map([&](const program &prog) {
>             return prog.binary(dev);
>          }, progs);
> 
> > +      clean(&dev, keep_log);
> > +
> > +      _opts.insert({ &dev, opts });
> > +
> > +      if (mods.size() == 0) {
> > +         append_to_log(&dev, "Nothing to link.");
> > +         r = false;
> > +         continue;
> > +      }
> > +
> 
> This seems redundant, it's the api layer's responsibility to catch cases
> in which zero program objects are being linked, and return
> CL_INVALID_VALUE.

First patch made more checking based on program binary type.
Anyway it doesn't need to be there for the moment.

> 
> > +      std::string log;
> > +
> > +      try {
> > +         auto module = link_program_llvm(mods,
> > +                                         dev.ir_format(),
> > dev.ir_target(),
> > +                                         opts, log);
> > +         _binaries.insert({ &dev, module });
> > +         append_to_log(&dev, log);
> > +      } catch (const link_option_error &) {
> > +         append_to_log(&dev, log);
> > +         throw;
> > +      } catch (const error &) {
> > +         append_to_log(&dev, log);
> > +         r = false;
> 
> I suggest you just catch "const error &", update the error log and
> rethrow here, so you save a catch block and an error subclass.

As explain clLinkProgram doesn't behave the same way regarding error during 
option parsing and after.

> 
> > +      }
> > +   }
> > +
> > +   return r;
> 
> Ugh...  This is mixing exception-based and return code-based error
> handling in the same function.  Please just throw link_error if anything
> goes wrong (what will also simplify all of its users because they will
> no longer have to check the return code explicitly).

ok

> 
> > +}
> > +
> > +void
> > +program::clean(const device *dev, bool keep_log) {
> > +   _binaries.erase(dev);
> > +   _opts.erase(dev);
> > +   if (!keep_log)
> > +      _logs.erase(dev);
> > +}
> 
> This method seems rather ad-hoc and doesn't look like it helps abstract
> any knowledge from the callers, because they need to manipulate the maps
> directly, and the different handling of build logs forced you to add a
> boolean argument that changes the semantics of the function specifically
> 
> for each use-case.  IMO code like the following:
> |      clean(dev, true);
> 
> is less obvious than the two lines of equivalent code (e.g. a casual
> reader might wonder what is being set to true, question that cannot be
> answered by looking at the caller alone).  I suggest you just open-code
> the function, it will be less LoC overall anyway.

ok

> 
> > +
> > +void
> > +program::append_to_log(const device *dev, const std::string &log) {
> > +   std::string &l = _logs[dev];
> > +   l.empty() ? l = log : l + "\n" + log;
> > +}
> > +
> 
> This seems wrong.  And why is it even necessary?  Couldn't you just do
> "_logs[&dev] += message;" for each caller?  The build logs should
> already have been newline-terminated, and if they weren't in some case
> it's hiding a bug somewhere else.

ok

> 
> >  const std::string &
> >  program::source() const {
> >  
> >     return _source;
> > 
> > diff --git a/src/gallium/state_trackers/clover/core/program.hpp
> > b/src/gallium/state_trackers/clover/core/program.hpp index
> > 183145e..314979b 100644
> > --- a/src/gallium/state_trackers/clover/core/program.hpp
> > +++ b/src/gallium/state_trackers/clover/core/program.hpp
> > @@ -47,8 +47,11 @@ namespace clover {
> > 
> >        program &
> >        operator=(const program &prog) = delete;
> > 
> > -      void build(const ref_vector<device> &devs, const char *opts,
> > -                 const header_map &headers = {});
> > +      void build(const ref_vector<device> &devs, const char *opts);
> > +      void compile(const ref_vector<device> &devs, const char *opts,
> > +                   const header_map &headers);
> > +      bool link(const ref_vector<device> &devs, const char *opts,
> > +                const ref_vector<program> &progs, bool keep_log = false);
> 
> How about we make opts a "const std::string &" for these methods?  It's
> what the LLVM interfacing code expects now that everything uses STL
> types consistently.

I was keeping the same signature, I will change that

> 
> >        const bool has_source;
> >        const std::string &source() const;
> > 
> > @@ -69,6 +72,9 @@ namespace clover {
> > 
> >        friend class kernel;
> >     
> >     private:
> > +      void clean(const device *dev, bool keep_log = false);
> > +      void append_to_log(const device *dev, const std::string &log);
> > +
> > 
> >        std::vector<intrusive_ref<device>> _devices;
> >        std::map<const device *, module> _binaries;
> >        std::map<const device *, std::string> _logs;
> 
> I've stopped reviewing at this point because the changes below seem like
> a considerable amount of rather complex churn that could have been done
> independently from the changes to the core layer.  Maybe split them as a
> separate patch?

I will made another version.

EdB


> 
> > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > b/src/gallium/state_trackers/clover/llvm/invocation.cpp index
> > 9b91fee..ce5e3f2 100644
> > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > @@ -108,7 +108,7 @@ namespace {
> > 
> >           name, llvm::MemoryBuffer::getMemBuffer(source));
> >        
> >        if (!c.ExecuteAction(act))
> > 
> > -         throw build_error(log);
> > +         throw compile_error(log);
> > 
> >     }
> >     
> >     module
> > 
> > @@ -130,22 +130,15 @@ namespace {
> > 
> >         }
> >     
> >     }
> > 
> > -   llvm::Module *
> > -   compile_llvm(llvm::LLVMContext &llvm_ctx, const std::string &source,
> > -                const header_map &headers,
> > -                const std::string &name, const std::string &triple,
> > -                const std::string &processor, const std::string &opts,
> > -                clang::LangAS::Map& address_spaces, unsigned
> > &optimization_level, -                std::string &r_log) {
> > +   bool
> > +   create_from_arg_llvm(clang::CompilerInstance &c,
> > +                        const std::string &target, const std::string
> > &opts, +                        llvm::raw_string_ostream &log) {
> > 
> > -      clang::CompilerInstance c;
> > -      clang::EmitLLVMOnlyAction act(&llvm_ctx);
> > -      std::string log;
> > -      llvm::raw_string_ostream s_log(log);
> > -      std::string libclc_path = LIBCLC_LIBEXECDIR + processor + "-"
> > -                                                  + triple + ".bc";
> > -
> > -      // Parse the compiler options:
> > +      // Parse the compiler options.
> > +      // A file name should be present at the end
> > +      // and must have the .cl extension in order for the
> > +      // CompilerInvocation class to recognize it as an OpenCL source
> > file.> 
> >        std::vector<std::string> opts_array;
> >        std::istringstream ss(opts);
> > 
> > @@ -155,8 +148,6 @@ namespace {
> > 
> >           opts_array.push_back(opt);
> >        
> >        }
> > 
> > -      opts_array.push_back(name);
> > -
> > 
> >        std::vector<const char *> opts_carray;
> >        for (unsigned i = 0; i < opts_array.size(); i++) {
> >        
> >           opts_carray.push_back(opts_array.at(i).c_str());
> > 
> > @@ -171,15 +162,59 @@ namespace {
> > 
> >        DiagsBuffer = new clang::TextDiagnosticBuffer();
> >        
> >        clang::DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagsBuffer);
> > 
> > -      bool Success;
> > 
> > +      bool Success;
> > 
> >        Success =
> >        clang::CompilerInvocation::CreateFromArgs(c.getInvocation(),
> >        
> >                                          opts_carray.data(),
> >                                          opts_carray.data() +
> >                                          opts_carray.size(),
> >                                          Diags);
> > 
> > -      if (!Success) {
> > +
> > +      if (Success) {
> > +         const size_t processor_str_len = target.find_first_of("-");
> > +         const std::string processor(target, 0, processor_str_len);
> > +         const std::string triple(target.begin() + processor_str_len + 1,
> > +                                  target.end());
> > +
> > +         c.getLangOpts().NoBuiltin = true;
> > +         c.getTargetOpts().Triple = triple;
> > +         c.getTargetOpts().CPU = processor;
> > +
> > +         // This is a workaround for a Clang bug which causes the number
> > +         // of warnings and errors to be printed to stderr.
> > +         // http://www.llvm.org/bugs/show_bug.cgi?id=19735
> > +         c.getDiagnosticOpts().ShowCarets = false;
> > +         c.getInvocation().setLangDefaults(c.getLangOpts(),
> > clang::IK_OpenCL, +                                       
> > clang::LangStandard::lang_opencl11); +         c.createDiagnostics(
> > +                             new clang::TextDiagnosticPrinter(
> > +                                    log, &c.getDiagnosticOpts())
> > +                            );
> > +
> > +        
> > c.setTarget(clang::TargetInfo::CreateTargetInfo(c.getDiagnostics(), +    
> >                                           c.getInvocation().TargetOpts));
> > +      }
> > +
> > +      return Success;
> > +   }
> > +
> > +   llvm::Module *
> > +   compile_llvm(llvm::LLVMContext &llvm_ctx, const std::string &source,
> > +                const header_map &headers,
> > +                const std::string &target, const std::string &opts,
> > +                clang::LangAS::Map& address_spaces, unsigned
> > &optimization_level, +                std::string &r_log) {
> > +
> > +      clang::CompilerInstance c;
> > +      clang::EmitLLVMOnlyAction act(&llvm_ctx);
> > +      std::string log;
> > +      llvm::raw_string_ostream s_log(log);
> > +      const std::string name = "compile.cl";
> > +
> > +      if (!create_from_arg_llvm(c, target, opts + " " + name, s_log)) {
> > +         r_log = log;
> > 
> >           throw error(CL_INVALID_COMPILER_OPTIONS);
> >        
> >        }
> > 
> > +
> > 
> >        c.getFrontendOpts().ProgramAction = clang::frontend::EmitLLVMOnly;
> >        c.getHeaderSearchOpts().UseBuiltinIncludes = true;
> >        c.getHeaderSearchOpts().UseStandardSystemIncludes = true;
> > 
> > @@ -196,22 +231,6 @@ namespace {
> > 
> >        // clc.h requires that this macro be defined:
> >        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifi
> >        ers");
> > 
> > -
> > -      c.getLangOpts().NoBuiltin = true;
> > -      c.getTargetOpts().Triple = triple;
> > -      c.getTargetOpts().CPU = processor;
> > -
> > -      // This is a workaround for a Clang bug which causes the number
> > -      // of warnings and errors to be printed to stderr.
> > -      // http://www.llvm.org/bugs/show_bug.cgi?id=19735
> > -      c.getDiagnosticOpts().ShowCarets = false;
> > -      c.getInvocation().setLangDefaults(c.getLangOpts(),
> > clang::IK_OpenCL,
> > -                                       
> > clang::LangStandard::lang_opencl11); -      c.createDiagnostics(
> > -                          new clang::TextDiagnosticPrinter(
> > -                                 s_log,
> > -                                 &c.getDiagnosticOpts()));
> > -
> > 
> >  #if HAVE_LLVM >= 0x0306
> >  
> >        c.getPreprocessorOpts().addRemappedFile(name,
> >        
> >                                                llvm::MemoryBuffer::getMemB
> >                                                uffer(source).release());
> > 
> > @@ -247,6 +266,7 @@ namespace {
> > 
> >        // attribute.  This attribute will prevent Clang from creating
> >        // illegal uses of barrier() (e.g. Moving barrier() inside a
> >        conditional
> >        // that is no executed by all threads) during its optimizaton
> >        passes.
> > 
> > +      const std::string libclc_path = LIBCLC_LIBEXECDIR + target + ".bc";
> > 
> >        c.getCodeGenOpts().LinkBitcodeFile = libclc_path;
> >        
> >        optimization_level = c.getCodeGenOpts().OptimizationLevel;
> > 
> > @@ -256,7 +276,7 @@ namespace {
> > 
> >        r_log = log;
> >        
> >        if (!ExecSuccess)
> > 
> > -         throw build_error();
> > +         throw compile_error();
> > 
> >        // Get address spaces map to be able to find kernel argument
> >        address space
> >        memcpy(address_spaces, c.getTarget().getAddressSpaceMap(),
> > 
> > @@ -485,7 +505,7 @@ namespace {
> > 
> >        LLVMDisposeMessage(err_message);
> >        
> >        if (err) {
> > 
> > -         throw build_error();
> > +         throw link_error();
> > 
> >        }
> >     
> >     }
> > 
> > @@ -505,7 +525,7 @@ namespace {
> > 
> >        if (LLVMGetTargetFromTriple(triple.c_str(), &target,
> >        &error_message)) {
> >        
> >           r_log = std::string(error_message);
> >           LLVMDisposeMessage(error_message);
> > 
> > -         throw build_error();
> > +         throw link_error();
> > 
> >        }
> >        
> >        LLVMTargetMachineRef tm = LLVMCreateTargetMachine(
> > 
> > @@ -514,7 +534,7 @@ namespace {
> > 
> >        if (!tm) {
> >        
> >           r_log = "Could not create TargetMachine: " + triple;
> > 
> > -         throw build_error();
> > +         throw link_error();
> > 
> >        }
> >        
> >        if (dump_asm) {
> > 
> > @@ -567,7 +587,7 @@ namespace {
> > 
> >              const char *name;
> >              if (gelf_getshdr(section, &symtab_header) != &symtab_header)
> >              {
> >              
> >                 r_log = "Failed to read ELF section header.";
> > 
> > -               throw build_error();
> > +               throw link_error();
> > 
> >              }
> >              name = elf_strptr(elf, section_str_index,
> >              symtab_header.sh_name);
> >             
> >             if (!strcmp(name, ".symtab")) {
> > 
> > @@ -577,9 +597,9 @@ namespace {
> > 
> >           }
> >           if (!symtab) {
> >           
> >              r_log = "Unable to find symbol table.";
> > 
> > -            throw build_error();
> > +            throw link_error();
> > 
> >           }
> > 
> > -      } catch (build_error &e) {
> > +      } catch (link_error &e) {
> > 
> >           elf_end(elf);
> >           throw e;
> >        
> >        }
> > 
> > @@ -640,6 +660,7 @@ namespace {
> > 
> >        return m;
> >     
> >     }
> > 
> > +   template<typename T>
> > 
> >     void
> >     diagnostic_handler(const llvm::DiagnosticInfo &di, void *data) {
> >     
> >        if (di.getSeverity() == llvm::DS_Error) {
> > 
> > @@ -650,7 +671,7 @@ namespace {
> > 
> >           stream.flush();
> >           *(std::string*)data = message;
> > 
> > -         throw build_error();
> > +         throw T();
> > 
> >        }
> >     
> >     }
> > 
> > @@ -689,38 +710,28 @@ namespace {
> > 
> >  module
> >  clover::compile_program_llvm(const std::string &source,
> > 
> > -                             const header_map &headers,
> > -                             enum pipe_shader_ir ir,
> > -                             const std::string &target,
> > -                             const std::string &opts,
> > -                             std::string &r_log) {
> > +                                const header_map &headers,
> > +                                const std::string &target,
> > +                                const std::string &opts,
> > +                                std::string &r_log) {
> > 
> >     init_targets();
> > 
> > -   std::vector<llvm::Function *> kernels;
> > -   size_t processor_str_len = std::string(target).find_first_of("-");
> > -   std::string processor(target, 0, processor_str_len);
> > -   std::string triple(target, processor_str_len + 1,
> > -                      target.size() - processor_str_len - 1);
> > 
> >     clang::LangAS::Map address_spaces;
> >     llvm::LLVMContext llvm_ctx;
> >     unsigned optimization_level;
> > 
> > -   llvm_ctx.setDiagnosticHandler(diagnostic_handler, &r_log);
> > +   llvm_ctx.setDiagnosticHandler(diagnostic_handler<compile_error>,
> > &r_log);> 
> >     if (get_debug_flags() & DBG_CLC)
> > 
> > -      debug_log("// Build options: " + opts + '\n' + source, ".cl");
> > +      debug_log("// options: " + opts + '\n' + source, ".cl");
> > 
> >     // The input file name must have the .cl extension in order for the
> >     // CompilerInvocation class to recognize it as an OpenCL source file.
> > 
> > -   llvm::Module *mod = compile_llvm(llvm_ctx, source, headers,
> > "input.cl",
> > -                                    triple, processor, opts,
> > address_spaces, +   llvm::Module *mod = compile_llvm(llvm_ctx, source,
> > headers,
> > +                                    target, opts, address_spaces,
> > 
> >                                      optimization_level, r_log);
> > 
> > -   find_kernels(mod, kernels);
> > -
> > -   optimize(mod, optimization_level, kernels);
> > -
> > 
> >     if (get_debug_flags() & DBG_LLVM) {
> >     
> >        std::string log;
> >        llvm::raw_string_ostream s_log(log);
> > 
> > @@ -729,25 +740,18 @@ clover::compile_program_llvm(const std::string
> > &source,> 
> >        debug_log(log, ".ll");
> >      
> >      }
> > 
> > +   //serialize for later use
> > 
> >     module m;
> > 
> > -   // Build the clover::module
> > -   switch (ir) {
> > -      case PIPE_SHADER_IR_TGSI:
> > -         //XXX: Handle TGSI
> > -         assert(0);
> > -         m = module();
> > -         break;
> > -      case PIPE_SHADER_IR_LLVM:
> > -         m = build_module_llvm(mod, kernels, address_spaces);
> > -         break;
> > -      case PIPE_SHADER_IR_NATIVE: {
> > -         std::vector<char> code = compile_native(mod, triple, processor,
> > -                                                 get_debug_flags() &
> > DBG_ASM, -                                                 r_log);
> > -         m = build_module_native(code, mod, kernels, address_spaces,
> > r_log); -         break;
> > -      }
> > -   }
> > +   llvm::SmallVector<char, 1024> llvm_bitcode;
> > +   llvm::raw_svector_ostream bitcode_ostream(llvm_bitcode);
> > +   llvm::BitstreamWriter writer(llvm_bitcode);
> > +   llvm::WriteBitcodeToFile(mod, bitcode_ostream);
> > +   bitcode_ostream.flush();
> > +
> > +   std::vector<char> data(llvm_bitcode.begin(), llvm_bitcode.end());
> > +   m.secs.push_back(module::section(0, module::section::text,
> > +                                       data.size(), data));
> > +
> > 
> >  #if HAVE_LLVM >= 0x0306
> >  
> >     // LLVM 3.6 and newer, the user takes ownership of the module.
> >     delete mod;
> > 
> > @@ -755,3 +759,120 @@ clover::compile_program_llvm(const std::string
> > &source,> 
> >     return m;
> >  
> >  }
> > 
> > +
> > +
> > +module
> > +clover::link_program_llvm(const std::vector<module> &modules,
> > +                             enum pipe_shader_ir ir,
> > +                             const std::string &target,
> > +                             const std::string &opts,
> > +                             std::string &r_log) {
> > +
> > +   init_targets();
> > +
> > +   std::string options = opts;
> > +   bool create_library = false;
> > +   size_t pos = options.find("-create-library");
> > +   if (pos != std::string::npos) {
> > +      create_library = true;
> > +      options.erase(pos, 15);
> > +   }
> > +   options += " link.cl";
> > +
> > +   llvm::LLVMContext llvm_ctx;
> > +   llvm_ctx.setDiagnosticHandler(diagnostic_handler<link_error>, &r_log);
> > +
> > +   std::string ci_log;
> > +   llvm::raw_string_ostream rso(ci_log);
> > +   clang::CompilerInstance c;
> > +   if (!create_from_arg_llvm(c, target, options, rso)) {
> > +      r_log = ci_log;
> > +      throw link_option_error();
> > +   }
> > +
> > +   llvm::Module linked_mod("link", llvm_ctx);
> > +   llvm::Linker linker(&linked_mod);
> > +
> > +   for (const auto &mod : modules) {
> > +      const auto s = llvm::StringRef(mod.secs[0].data.data(),
> > +                                                    
> > mod.secs[0].data.size()); +
> > +      auto m =
> > +         llvm::parseBitcodeFile(llvm::MemoryBufferRef(s, " "), llvm_ctx);
> > +
> > +      if (!m) {
> > +         r_log = m.getError().message();
> > +         throw error(CL_INVALID_PROGRAM);
> > +      }
> > +
> > +#if HAVE_LLVM < 0x0306
> > +      if (linker.linkInModule(*m, &r_log))
> > +#elif HAVE_LLVM < 0x0307
> > +      if (linker.linkInModule(*m))
> > +#else
> > +      if (linker.linkInModule((*m).get()))
> > +#endif
> > +         throw link_error();
> > +   }
> > +
> > +   module m;
> > +   std::vector<llvm::Function *> kernels;
> > +
> > +   //optimize
> > +   if (!create_library)
> > +      find_kernels(&linked_mod, kernels);
> > +
> > +   unsigned optimization_level = c.getCodeGenOpts().OptimizationLevel;
> > +   optimize(&linked_mod, optimization_level, kernels);
> > +
> > +
> > +   unsigned debug_flags = get_debug_flags();
> > +
> > +   if (debug_flags & DBG_LLVM) {
> > +      std::string log;
> > +      llvm::raw_string_ostream s_log(log);
> > +      linked_mod.print(s_log, NULL);
> > +      s_log.flush();
> > +      debug_log(log, ".ll");
> > +    }
> > +
> > +    if (create_library) {
> > +      //serialize for later use
> > +      llvm::SmallVector<char, 1024> llvm_bitcode;
> > +      llvm::raw_svector_ostream bitcode_ostream(llvm_bitcode);
> > +      llvm::BitstreamWriter writer(llvm_bitcode);
> > +      llvm::WriteBitcodeToFile(&linked_mod, bitcode_ostream);
> > +      bitcode_ostream.flush();
> > +
> > +      std::vector<char> data(llvm_bitcode.begin(), llvm_bitcode.end());
> > +      m.secs.push_back(module::section(0, module::section::text,
> > +                                       data.size(), data));
> > +
> > +   } else {
> > +      // Get address spaces map to be able to find kernel argument
> > address space +      clang::LangAS::Map address_spaces;
> > +      memcpy(address_spaces, c.getTarget().getAddressSpaceMap(),
> > +                                                       
> > sizeof(address_spaces)); +
> > +      // Build the clover::module
> > +      switch (ir) {
> > +         case PIPE_SHADER_IR_TGSI:
> > +            assert(0); // not supported
> > +            break;
> > +         case PIPE_SHADER_IR_LLVM:
> > +            m = build_module_llvm(&linked_mod, kernels, address_spaces);
> > +            break;
> > +         case PIPE_SHADER_IR_NATIVE: {
> > +            auto code = compile_native(&linked_mod,
> > c.getTargetOpts().Triple, +                                         
> > c.getTargetOpts().CPU,
> > +                                          debug_flags & DBG_ASM, r_log);
> > +            m = build_module_native(code, &linked_mod, kernels,
> > address_spaces, +                                    r_log);
> > +            break;
> > +         }
> > +      }
> > +
> > +   }
> > +
> > +   return m;
> > +}



More information about the mesa-dev mailing list