[Mesa-stable] [Mesa-dev] [PATCH 1/2] gallivm: Allow drivers and state trackers to initialize gallivm LLVM targets

[Tom Stellard]

On Thu, Sep 24, 2015 at 10:48:32PM +0100, Emil Velikov wrote:
> Hi Tom,
> 
> On 24 September 2015 at 17:31, Tom Stellard <thomas.stellard at amd.com> wrote:
> > Drivers and state trackers that use LLVM for generating code, must
> > register the targets they use with LLVM's global TargetRegistry.
> > The TargetRegistry is not thread-safe, so all targets must be added
> > to the registry before it can be queried for target information.
> >
> > When drivers and state trackers initialize their own targets, they need
> > a way to force gallivm to initialize its targets at the same time.
> > Otherwise, there can be a race condition in some multi-threaded
> > applications (e.g. glx-multihreaded-shader-compile in piglit),
> > when one thread creates a context for a driver that uses LLVM (e.g.
> > radeonsi) and another thread creates a gallivm context (glxContextCreate
> > does this).
> >
> > The race happens when the driver thread initializes its LLVM targets and
> > then starts using the registry before the gallivm thread has a chance to
> > register its targets.
> >
> > This patch allows users to force gallivm to register its targets by
> > adding two new functions:  gallivm_llvm_init_begin() which locks a mutex
> > guarding the TargetRegistry and initializes the gallivm targets, and
> > gallivm_llvm_init_end() which unlocks the mutex.
> >
> > Drivers and state trackers should use this sequence to correctly
> > initialize the LLVM targets they use:
> >
> > gallivm_init_llvm_begin();
> > LLVMInitializeFooTarget();
> > gallivm_init_llvm_end();
> >
> > CC: "10.6 11.0" <mesa-stable at lists.freedesktop.org>
> > ---
> >  src/gallium/auxiliary/gallivm/lp_bld_misc.cpp | 68 ++++++++++++++++++++++++---
> >  src/gallium/auxiliary/gallivm/lp_bld_misc.h   |  5 ++
> >  2 files changed, 66 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> > index 5e25819..8bf833e 100644
> > --- a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> > +++ b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
> > @@ -81,6 +81,7 @@
> >  #  pragma pop_macro("DEBUG")
> >  #endif
> >
> > +#include "os/os_thread.h"
> >  #include "pipe/p_config.h"
> >  #include "util/u_debug.h"
> >  #include "util/u_cpu_detect.h"
> > @@ -101,6 +102,64 @@ public:
> >
> >  static LLVMEnsureMultithreaded lLVMEnsureMultithreaded;
> >
> > +struct LLVMRegistryLock {
> > +private:
> > +   pipe_mutex mutex;
> > +
> > +public:
> > +   LLVMRegistryLock()
> > +   {
> > +      pipe_mutex_init(mutex);
> > +   }
> > +
> > +   void lock()
> > +   {
> > +      pipe_mutex_lock(mutex);
> > +   }
> > +
> > +   void unlock()
> > +   {
> > +      pipe_mutex_unlock(mutex);
> > +   }
> > +};
> > +
> > +static LLVMRegistryLock gallivm_llvm_registry_lock;
> > +
> > +}
> > +
> > +/**
> > + * The llvm target registry is not thread-safe, so drivers and state-trackers
> > + * that want to initialize targets should use the gallivm_init_llvm_begin() and
> > + * gallivm_init_llvm_end() functions to safely initialize targets.  For example:
> > + *
> > + * gallivm_init_llvm_begin();
> > + * LLVMInitializeFooTarget();
> > + * gallivm_init_llvm_end();
> > + *
> > + * LLVM targets should be initialized before the driver tries to access the
> > + * registry.
> > + */
> > +extern "C" void
> > +gallivm_init_llvm_begin(void)
> > +{
> > +   static int init = 0;
> > +   gallivm_llvm_registry_lock.lock();
> > +   if (!init) {
> > +      // If we have a native target, initialize it to ensure it is linked in and
> > +      // usable by the JIT.
> > +      llvm::InitializeNativeTarget();
> > +
> > +      llvm::InitializeNativeTargetAsmPrinter();
> > +
> > +      llvm::InitializeNativeTargetDisassembler();
> > +      init = 1;
> > +    }
> > +}
> > +
> > +extern "C" void
> > +gallivm_init_llvm_end(void)
> > +{
> > +   gallivm_llvm_registry_lock.unlock();
> >  }
> >
> >  extern "C" void
> > @@ -115,13 +174,8 @@ lp_set_target_options(void)
> >     llvm::DisablePrettyStackTrace = true;
> >  #endif
> >
> > -   // If we have a native target, initialize it to ensure it is linked in and
> > -   // usable by the JIT.
> > -   llvm::InitializeNativeTarget();
> > -
> > -   llvm::InitializeNativeTargetAsmPrinter();
> > -
> > -   llvm::InitializeNativeTargetDisassembler();
> > +   gallivm_init_llvm_begin();
> > +   gallivm_init_llvm_end();
> >  }
> >
> What is the advantage of these wrappers instead of the (more obvious imho)
> 
> pipe_mutex_init(mutex); // wrap in call_once() ?

I think this is a C11/C++11 feature.  I forget whether or not these are
allowed.

> pipe_mutex_lock(mutex);
> 
> llvm::InitializeNativeTarget...
> 
> pipe_mutex_unlock(mutex);
> 
> By adding these you effectively have extra calls to
> llvm::InitializeNativeTarget* for radeon. The second patch does not
> mention anything on the topic, is that intentional ?
> 

It is intentional to have radeon call llvm::InitializeNativeTarget*.
radeon needs to preemptively initialize gallivm's targets to avoid the
race condition.

-Tom

> Cheers,
> Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev