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

[Emil Velikov]

On 25 September 2015 at 14:55, Tom Stellard <tom at stellard.net> wrote:
> 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.
>
We've had the c11/threads compatible implementation in
$mesa_top/include for quite a while now. We've even moved most of
classic mesa, mapi, egl, etc. to use it.

>> 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.
>
Hmm indeed. I've assumed that radeon explicitly calls this somehow,
but it seems that most/all users are in aux/draw.

Thanks
Emil