[Mesa-dev] [PATCH] radeon/llvm: Use LLVM C API for compiling LLVM IR to ISA.

[Jose Fonseca]

----- Original Message -----
> 
> Jose,
> 
> On Thursday, April 25, 2013 01:38:46 Jose Fonseca wrote:
> > What I'm suggesting doesn't require huge effort.
> > 
> > In detail, I'm suggesting:
> > 
> > (1) have a custom build of LLVM libraries with -fvisibility=hidden
> > 
> > (2) have a src/mesallvm/mesallvm.c containing wrappers
> > 
> >   #include <llvm-c/Core.h>
> > 
> > __attribute__((visibility("default")))
> >   LLVMValueRef
> >   MesaLLVMBuildAdd(LLVMBuilderRef Builder, LLVMValueRef LHS, LLVMValueRef
> > RHS, const char *Name) { return LLVMBuildAdd(Builder, LHS, RHS, Name);
> >   }
> > 
> >   ...
> > 
> >   plus a src/mesallvm/mesallvm.cpp with whatever custom C wrappers to C++
> > functionality we need (e.g, the stuff in lp_build_misc.cpp).
> > 
> >   These two modules would be statically linked against the custom LLVM
> > libraries from (1), producing a shared library libMESALLVM.so, which would
> > be consumed by any Mesa driver that needs it. Because all symbols have the
> > unique MESALLVM prefix, these should not collide with user stuff.
> > 
> > (3) have a src/mesallvm/mesallvm.h
> > 
> >    // Mange LLVM symbols before including them
> >    #define LLVMBuildAdd MesaLLVMBuildAdd
> > 
> >    ...
> > 
> >    #include <llvm-c/Core.h>
> >    ...
> > 
> >    All gallium code would include src/mesallvm/mesallvm.h instead. It can
> > still use the LLVMFoo as befor.
> > 
> > And that's all there is to it.
> 
> Technically this will work.
> 
> But I fear that this build procedure is too non standard to be really picked
> up by the distribution builders. They would need to build a seperate llvm
> version inside of the mesa package. Doable, but do you think we can convince
> them all?

This is for you and them to decide. I have no experience nor opinion on that.

> The technical problem you try to solve is that we need to force the llvm
> build
> to have the default hidden visibility.
> I think we could achieve the same result by using a default built llvm static
> lib and explicitly mention the to be exported symbols in a linker script.
> IIRC
> we could even use wildcards there. That is only export everything starting
> with MesaLLVM*.
> 
> Remains that we need to write out all the wrappers, which I called 'huge
> effort'.

I think our scales are greatness are too different:

- apitrace has to deal with 2861 entry points

- LLVM C bindings has 588

  $ grep -ro '\<LLVM\w\+(' /usr/lib/llvm-3.1/include/llvm-c/* | sort -u | wc -l
  588

- and mesa uses only 186 of them:

  $ git grep '\<LLVM\w\+(' | grep -o '\<LLVM\w\+(' | sort -u  | wc -l
  186

That is, it would probably take me 1hr max to write a script to extract all these symbols into wrappers.

> May be this could equally well done by looking with scripts at the llvm
> libraries and create an apropriate version script. Hide all symbols, except
> the ones starting with MesaLLVM and for each LLVM c symbol create an alias
> with the apropriate MesaLLVM symbol. Requires some shell or python hackery to
> generate the version script given the llvm c binding library.

If it works, sounds great by me. Feel free to ignore my suggestion.

My main concern is foremost what happens gallivm, and not so much the symbol problem. The motivation of my suggestions was merely to provide alternatives to those I foresee problematic.

> > > Again OTOH, the dlopen flags are really made for this kind of module use
> > > I
> > > think ...
> 
> I was referencing the patches that i just packed into the mail yesterday.
> This
> RTLD_LOCAL|RTLD_DEEPBIND flag already does what we need regarding isolation.
> This does not even require more changes than changing a bunch of dlopen flags
> -
> no wrappers to program.
> What I am not sure about is if we isolate too much for mesa/dri internal
> needs
> with this.

I confess I haven't had opportunity to look at them yet. I'm not familiar with the semantics of RTLD_DEEPBIND neither, so I can't comment.

> > Nevertheless, I must insist that the problem of hiding LLVM symbols is
> > handled in a layer underneath gallivm.  Severing gallivm module from the
> > rest auxiliary gallium modules, or making gallivm a full wrapper for all
> > LLVM symbols, seems unsustainable / too limiting IMO.
> Huch, then I probably do not get you right.
> Aren't you proposing a full wrapper of all the llvm c symbols by MesaLLVM*
> symbols above? Here you are calling exactly the same too limiting?

There is no paradox.  To be clear:

 - Severing gallivm from rest of auxiliary modules is too limiting -- gallivm needs those modules. The wrappers I recommmended are self sufficient. They are in no way limited by being isolated from other gallium auxiliary modules.

 - making gallivm a wrapper for all symbols is unsustainable, ugly. Take a look at gallivm abstraction for add operation:

/**
 * Generate a + b
 */
LLVMValueRef
lp_build_add(struct lp_build_context *bld,
             LLVMValueRef a,
             LLVMValueRef b)
{
   LLVMBuilderRef builder = bld->gallivm->builder;
   const struct lp_type type = bld->type;
   LLVMValueRef res;

   assert(lp_check_value(type, a));
   assert(lp_check_value(type, b));

   if(a == bld->zero)
      return b;
   if(b == bld->zero)
      return a;
   if(a == bld->undef || b == bld->undef)
      return bld->undef;

   if(bld->type.norm) {
      const char *intrinsic = NULL;

      if(a == bld->one || b == bld->one)
        return bld->one;

      if (type.width * type.length == 128 &&
          !type.floating && !type.fixed) {
         if(util_cpu_caps.has_sse2) {
           if(type.width == 8)
             intrinsic = type.sign ? "llvm.x86.sse2.padds.b" : "llvm.x86.sse2.paddus.b";
           if(type.width == 16)
             intrinsic = type.sign ? "llvm.x86.sse2.padds.w" : "llvm.x86.sse2.paddus.w";
         } else if (util_cpu_caps.has_altivec) {
           if(type.width == 8)
              intrinsic = type.sign ? "llvm.ppc.altivec.vaddsbs" : "llvm.ppc.altivec.vaddubs";
           if(type.width == 16)
              intrinsic = type.sign ? "llvm.ppc.altivec.vaddsws" : "llvm.ppc.altivec.vadduws";
         }
      }
   
      if(intrinsic)
         return lp_build_intrinsic_binary(builder, intrinsic, lp_build_vec_type(bld->gallivm, bld->type), a, b);
   }

   if(LLVMIsConstant(a) && LLVMIsConstant(b))
      if (type.floating)
         res = LLVMConstFAdd(a, b);
      else
         res = LLVMConstAdd(a, b);
   else
      if (type.floating)
         res = LLVMBuildFAdd(builder, a, b, "");
      else
         res = LLVMBuildAdd(builder, a, b, "");

   /* clamp to ceiling of 1.0 */
   if(bld->type.norm && (bld->type.floating || bld->type.fixed))
      res = lp_build_min_simple(bld, res, bld->one);

   /* XXX clamp to floor of -1 or 0??? */

   return res;
}

This does not suit most GPUs, which is why R600 tends to avoid gallivm's lp_bld_* abstractions and instead us LLVMBuild* functions firectly.  Therefore, in order to gallivm to be what you are proposing to be, it would need to add the wrappers, just like MesaLLVMBuildAdd. So if, new code will need to be written, then it should be in a separate layer.

- Finally, I must retain the ability to statically link LLVM (via configure/build option), so a separate layer would make it more easily to slot-in/out as needed.


Jose