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

[Emil Velikov]

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() ?
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 ?

Cheers,
Emil