[Beignet] [PATCH] Change clang system call to libclang api call.

Yang, Rong R rong.r.yang at intel.com
Wed May 22 22:48:52 PDT 2013


According to your suggestion, I have send new two patches, one patch is change clang system call to libclang api call, the other is enable llvm3.1 and fix llvm3.1 build errors.

-----Original Message-----
From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com] 
Sent: Thursday, May 23, 2013 10:18 AM
To: Yang, Rong R
Cc: beignet at lists.freedesktop.org
Subject: Re: [Beignet] [PATCH] Change clang system call to libclang api call.

On Wed, May 22, 2013 at 04:32:16AM +0000, Yang, Rong R wrote:
> 
> 
> -----Original Message-----
> From: beignet-bounces+rong.r.yang=intel.com at lists.freedesktop.org 
> [mailto:beignet-bounces+rong.r.yang=intel.com at lists.freedesktop.org] 
> On Behalf Of Zhigang Gong
> Sent: Tuesday, May 21, 2013 4:41 PM
> To: Yang, Rong R
> Cc: beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH] Change clang system call to libclang api call.
> 
> I tested this patch with LLVM3.2, and it works fine. As to the clang/llvm 3.1, I haven't tested it, but I have some minor comments as below, please check it out.
> Thanks. 
> 
> On Tue, May 21, 2013 at 12:45:56PM +0800, Yang Rong wrote:
> > The original call clang command directly as frontend. The implement is not very flexible.
> > I change to call libclang apis, now support both clang 3.1 and clang 3.2.
> > Now still write the intermediate to the file, for code simply.
> > Also fix llvm 3.1 build errors for my vector scalarize commit.
> > 
> > Signed-off-by: Yang Rong <rong.r.yang at intel.com>
> > ---
> >  CMake/FindLLVM.cmake                  |   27 ++++++++-
> >  backend/src/CMakeLists.txt            |    1 +
> >  backend/src/backend/program.cpp       |  103 ++++++++++++++++++++++++++-------
> >  backend/src/llvm/llvm_gen_backend.cpp |    7 +--
> >  backend/src/llvm/llvm_scalarize.cpp   |   10 ++--
> >  5 files changed, 116 insertions(+), 32 deletions(-)
> > 
> > diff --git a/CMake/FindLLVM.cmake b/CMake/FindLLVM.cmake index
> > c06b8a4..b320639 100644
> > --- a/CMake/FindLLVM.cmake
> > +++ b/CMake/FindLLVM.cmake
> > @@ -8,7 +8,7 @@
> >  # LLVM_FOUND       - True if llvm found.
> >  if (LLVM_INSTALL_DIR)
> >    find_program(LLVM_CONFIG_EXECUTABLE NAMES llvm-config-32 llvm-config-3.2 llvm-config DOC "llvm-config executable" PATHS ${LLVM_INSTALL_DIR} NO_DEFAULT_PATH)
> > -else (LLVM_INSTALL_DIR)                                                                
> > +else (LLVM_INSTALL_DIR)
> If we really want to support llvm 3.1, I think we need to add llvm-config-31 llvm-config-3.1 here. Right? And at the source root directory's CMakeLists.txt, we need to change the Find_Package(LLVM 3.2) to Find_Package(LLVM 3.1). Then any llvm version larger than 3.1 will satisfy the checking, otherwise, it fails with only llvm 3.1 installed.
> 
> Yes. The changes used to test llvm 3.1 is not include in this patch. If also need that patch, I will send it later.
That's fine, please submit that patch when you finish it.
> 
> >    find_program(LLVM_CONFIG_EXECUTABLE NAMES llvm-config-32
> > llvm-config-3.2 llvm-config DOC "llvm-config executable")  endif
> > (LLVM_INSTALL_DIR)
> >  
> > @@ -66,3 +66,28 @@ execute_process(
> >    OUTPUT_VARIABLE LLVM_MODULE_LIBS
> >    OUTPUT_STRIP_TRAILING_WHITESPACE
> >  )
> > +
> > +macro(add_one_lib name)
> > +  FIND_LIBRARY(CLANG_LIB
> > +    NAMES ${name}
> > +    PATHS ${LLVM_LIBRARY_DIR} )
> > +  set(CLANG_LIBRARIES ${CLANG_LIBRARIES} ${CLANG_LIB})
> > +	unset(CLANG_LIB CACHE)
> > +endmacro()
> > +
> > +#Assume clang lib path same as llvm lib path
> > +add_one_lib("clangFrontend")
> > +add_one_lib("clangSerialization")
> > +add_one_lib("clangDriver")
> > +add_one_lib("clangCodeGen")
> > +add_one_lib("clangSema")
> > +add_one_lib("clangStaticAnalyzerFrontend")
> > +add_one_lib("clangStaticAnalyzerCheckers")
> > +add_one_lib("clangStaticAnalyzerCore")
> > +add_one_lib("clangAnalysis")
> > +add_one_lib("clangEdit")
> > +add_one_lib("clangAST")
> > +add_one_lib("clangParse")
> > +add_one_lib("clangSema")
> > +add_one_lib("clangLex")
> > +add_one_lib("clangBasic")
> > diff --git a/backend/src/CMakeLists.txt b/backend/src/CMakeLists.txt 
> > index 183517a..a0fe198 100644
> > --- a/backend/src/CMakeLists.txt
> > +++ b/backend/src/CMakeLists.txt
> > @@ -116,6 +116,7 @@ target_link_libraries(
> >                        ${DRM_INTEL_LIBRARY}
> >                        ${DRM_LIBRARY}
> >                        ${OPENGL_LIBRARIES}
> > +                      ${CLANG_LIBRARIES}
> >                        ${LLVM_MODULE_LIBS}
> >                        ${CMAKE_THREAD_LIBS_INIT}
> >                        ${CMAKE_DL_LIBS}) diff --git 
> > a/backend/src/backend/program.cpp b/backend/src/backend/program.cpp 
> > index c46c681..6816a13 100644
> > --- a/backend/src/backend/program.cpp
> > +++ b/backend/src/backend/program.cpp
> > @@ -46,6 +46,23 @@
> >  #define LLVM_VERSION_MINOR 0
> >  #endif /* !defined(LLVM_VERSION_MINOR) */
> >  
> > +#include <clang/CodeGen/CodeGenAction.h> #include 
> > +<clang/Frontend/CompilerInstance.h>
> > +#include <clang/Frontend/CompilerInvocation.h>
> > +#if LLVM_VERSION_MINOR <= 1
> > +#include <clang/Frontend/DiagnosticOptions.h>
> > +#else
> > +#include <clang/Basic/DiagnosticOptions.h> #endif  /* 
> > +LLVM_VERSION_MINOR <= 1 */ #include 
> > +<clang/Frontend/TextDiagnosticPrinter.h>
> > +#include <clang/Basic/TargetInfo.h> #include 
> > +<clang/Basic/TargetOptions.h> #include 
> > +<llvm/ADT/IntrusiveRefCntPtr.h> #include <llvm/ADT/OwningPtr.h> 
> > +#include <llvm/Module.h> #include <llvm/Bitcode/ReaderWriter.h> 
> > +#include <llvm/Support/raw_ostream.h>
> > +
> >  namespace gbe {
> >  
> >    Kernel::Kernel(const std::string &name) :
> > @@ -104,6 +121,71 @@ namespace gbe {
> >      GBE_SAFE_DELETE(program);
> >    }
> >  
> > +  static void buildModuleFromSource(const char* input, const char* output) {
> > +    // Arguments to pass to the clang frontend
> > +    vector<const char *> args;
> > +    args.push_back("-emit-llvm");
> > +    args.push_back("-O3");
> > +    args.push_back("-triple");
> > +    args.push_back("nvptx");
> > +    args.push_back(input);
> > +
> > +    // The compiler invocation needs a DiagnosticsEngine so it can 
> > +report problems #if LLVM_VERSION_MINOR <= 1
> > +    args.push_back("-triple");
> > +    args.push_back("ptx32");
> > +
> > +    clang::TextDiagnosticPrinter *DiagClient =
> > +                             new clang::TextDiagnosticPrinter(llvm::errs(), clang::DiagnosticOptions());
> > +    llvm::IntrusiveRefCntPtr<clang::DiagnosticIDs> DiagID(new clang::DiagnosticIDs());
> > +    clang::DiagnosticsEngine Diags(DiagID, DiagClient); #else
> > +    args.push_back("-ffp-contract=off");
> > +    args.push_back("-triple");
> > +    args.push_back("nvptx");
> > +
> > +    llvm::IntrusiveRefCntPtr<clang::DiagnosticOptions> DiagOpts = new clang::DiagnosticOptions();
> > +    clang::TextDiagnosticPrinter *DiagClient =
> > +                             new clang::TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
> > +    llvm::IntrusiveRefCntPtr<clang::DiagnosticIDs> DiagID(new clang::DiagnosticIDs());
> > +    clang::DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagClient); 
> > +#endif /* LLVM_VERSION_MINOR <= 1 */
> > +
> > +    // Create the compiler invocation
> > +    llvm::OwningPtr<clang::CompilerInvocation> CI(new clang::CompilerInvocation);
> > +    clang::CompilerInvocation::CreateFromArgs(*CI,
> > +                                              &args[0],
> > +                                              &args[0] + args.size(),
> > +                                              Diags);
> > +
> > +    // Create the compiler instance
> > +    clang::CompilerInstance Clang;
> > +    Clang.setInvocation(CI.take());
> > +    // Get ready to report problems
> > +    Clang.createDiagnostics(args.size(), &args[0]);
> > +    if (!Clang.hasDiagnostics())
> > +      return;
> > +
> > +    // Set Language
> > +    clang::LangOptions & lang_opts = Clang.getLangOpts();
> > +    lang_opts.OpenCL = 1;
> > +
> > +    // Create an action and make the compiler instance carry it out
> > +    llvm::OwningPtr<clang::CodeGenAction> Act(new clang::EmitLLVMOnlyAction());
> > +    if (!Clang.ExecuteAction(*Act))
> > +      return;
> > +
> > +    llvm::Module *module = Act->takeModule();
> > +
> > +    std::string ErrorInfo;
> > +    llvm::raw_fd_ostream OS(output, ErrorInfo,llvm::raw_fd_ostream::F_Binary);
> > +    //still write to temp file for code simply, otherwise need add another function.
> > +    //because gbe_program_new_from_llvm also be used by cl_program_create_from_llvm, can't be removed
> > +    //TODO: Pass module to llvmToGen, if use module, should return Act and use OwningPtr out of this funciton
> > +    llvm::WriteBitcodeToFile(module, OS);
> > +    OS.close();
> > +  }
> > +
> >    extern std::string ocl_stdlib_str;
> >    extern std::string ocl_common_defines_str;
> >    static gbe_program programNewFromSource(const char *source, @@
> > -124,26 +206,7 @@ namespace gbe {
> >      fwrite(source, strlen(source), 1, clFile);
> >      fclose(clFile);
> >  
> > -    // Now compile the code to llvm using clang
> > -#if LLVM_VERSION_MINOR <= 1
> > -    std::string compileCmd = "clang -x cl -fno-color-diagnostics -emit-llvm -O3 -ccc-host-triple ptx32 -c ";
> > -#else
> > -    std::string compileCmd = "clang -ffp-contract=off -emit-llvm -O3 -target nvptx -x cl -c ";
> > -#endif /* LLVM_VERSION_MINOR <= 1 */
> > -    compileCmd += clName;
> > -    compileCmd += " ";
> > -    if(options)
> > -      compileCmd += options;
> > -    compileCmd += " -o ";
> > -    compileCmd += llName;
> > -
> > -    // Open a pipe and compile from here. Using Clang API instead is better
> > -    FILE *pipe = popen(compileCmd.c_str(), "r");
> > -    FATAL_IF (pipe == NULL, "Unable to run extern compilation command");
> > -    char msg[256];
> > -    while (fgets(msg, sizeof(msg), pipe))
> > -      std::cout << msg;
> > -    pclose(pipe);
> > +    buildModuleFromSource(clName.c_str(), llName.c_str());
> >      remove(clName.c_str());
> >  
> >      // Now build the program from llvm diff --git 
> > a/backend/src/llvm/llvm_gen_backend.cpp
> > b/backend/src/llvm/llvm_gen_backend.cpp
> > index deda687..d9a773a 100644
> > --- a/backend/src/llvm/llvm_gen_backend.cpp
> > +++ b/backend/src/llvm/llvm_gen_backend.cpp
> > @@ -853,11 +853,8 @@ namespace gbe
> >        // Insert a new register for each function argument  #if 
> > LLVM_VERSION_MINOR <= 1
> >        const AttrListPtr &PAL = F.getAttributes();
> > -      uint32_t argID = 1; // Start at one actually
> So the previous implementation assign arg ID starting from 1 for the version prior to 3.2, right?
> This seems strange for me. I checked the code and found it was modified to start from 0 by Boqun for the LLVM version after 3.2 at commit 438c798.... Boqun, could you give some comments here?
> 
> Before Boqun's commit, it will build error with 3.1. Boqun set argID from 1 because class AttrListPtr's first attribute is return attribute, and argument from index 1.
Right. But the issue I talked about here is that this commit seems break Boqun's code.
You can see that you remove the above line of code which makes argID starting from 0 rather than 1.
I noticed that you change argID to argID+1 in one of the consequent code:
 -          if (PAL.paramHasAttr(argID, Attribute::ByVal)) {
 +          if (PAL.paramHasAttr(argID+1, Attribute::ByVal)) {
And that's fine.
But you miss one line of code:

          for(uint32_t i=1; i < elemNum; i++) {
            ir::PushLocation argLocation(fn, argID, elemSize*i);
            reg = getRegister(I, i);

In the above code, you are processing vector function argument's non-leading elements.
And associate those the registers with the same argID. Now, with this patch and llvm3.1, the argID becomes starting from 0 rather than from 1. Is that what you expect for? 

> 
> > -      for (; I != E; ++I, ++argID) {
> > -#else
> > -      for (; I != E; ++I, ++argID) {
> >  #endif /* LLVM_VERSION_MINOR <= 1 */
> > +      for (; I != E; ++I, ++argID) {
> >          const std::string &argName = I->getName().str();
> >          Type *type = I->getType();
> >  
> > @@ -892,7 +889,7 @@ namespace gbe
> >            PointerType *pointerType = dyn_cast<PointerType>(type);
> >            // By value structure
> >  #if LLVM_VERSION_MINOR <= 1
> > -          if (PAL.paramHasAttr(argID, Attribute::ByVal)) {
> > +          if (PAL.paramHasAttr(argID+1, Attribute::ByVal)) {
> >  #else
> >            if (I->hasByValAttr()) {
> >  #endif /* LLVM_VERSION_MINOR <= 1 */ diff --git 
> > a/backend/src/llvm/llvm_scalarize.cpp
> > b/backend/src/llvm/llvm_scalarize.cpp
> > index f71401f..ef431f2 100644
> > --- a/backend/src/llvm/llvm_scalarize.cpp
> > +++ b/backend/src/llvm/llvm_scalarize.cpp
> > @@ -71,7 +71,11 @@
> >  #include "llvm/IntrinsicInst.h"
> >  #include "llvm/Module.h"
> >  #include "llvm/Pass.h"
> > +#if LLVM_VERSION_MINOR <= 1
> > +#include "llvm/Support/IRBuilder.h"
> > +#else
> >  #include "llvm/IRBuilder.h"
> > +#endif /* LLVM_VERSION_MINOR <= 1 */
> >  #include "llvm/Support/CallSite.h"
> >  #include "llvm/Support/CFG.h"
> >  #include "llvm/Support/raw_ostream.h"
> > @@ -730,13 +734,7 @@ namespace gbe {
> >  
> >      Function::arg_iterator I = F.arg_begin(), E = F.arg_end();
> >  
> > -#if LLVM_VERSION_MINOR <= 1
> > -    const AttrListPtr &PAL = F.getAttributes();
> > -    uint32_t argID = 1; // Start at one actually
> > -    for (; I != E; ++I, ++argID) {
> > -#else
> >      for (; I != E; ++I) {
> > -#endif /* LLVM_VERSION_MINOR <= 1 */
> >        Type *type = I->getType();
> >  
> >        if(type->isVectorTy())
> > --
> > 1.7.9.5
> > 
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet


More information about the Beignet mailing list