[Mesa-dev] [PATCH] gallivm: correctly manage MCJIT at run-time

[Jose Fonseca]

I suspect this might break builds with LLVM 3.6 or higher.

The LLVMLinkInJIT must be inside #if ... #endif, and it must not be 
expanded when HAVE_LLVM >= 0x0306, since LLVMLinkInJIT(),

That is, when HAVE_LLVM >= 0x0306:
- USE_MCJIT should be static const
- no point claling GALLIVM_MCJIT
- must not have any LLVMLinkInJIT() call around, regardless it's called 
or not.


And this code doesn't make sense:

  if (USE_MCJIT)
     LLVMLinkInMCJIT();
  else
     LLVMLinkInJIT();

If these functions are meant to force the static linking of external 
libraries, putting any control flow around it is just misleading.  It 
gives the illusion that if we don't call these functions nothing will 
happen which is defintely not true.


 > As an added bonus might even solve the issue Wu Zhen is hitting :-)

I'm not enterly sure I understad Wu Zhen problem.

If android doesn't have MCJIT then I think the right fix is merely

#if defined(ANDROID)
#define USE_MCJIT 0
#endif

...


   // Link MCJIT when USE_MCJIT is a runtime option or defined as 1
#if !defined(USE_MCJIT) || USE_MCJIT
   LLVMLinkInMCJIT();
#endif

   // Link old JIT when USE_MCJIT is a runtime option or defined as 0
#if !defined(USE_MCJIT) || !USE_MCJIT
   LLVMLinkInJIT();
#endif

That is, any logic to decide whether to call or not LLVMLinkIn* must be 
done with _build_ time C-processor logic.


Jose


On 14/01/17 01:31, Emil Velikov wrote:
> Earlier commit made the decision whether to use MCJIT a run-time one. At
> the same time, it changed the code-flow in the following manner:
>  - LLVMLinkInMCJIT() was executed regardless of whether MCJIT is to be
> used or not. Admittedly it is a no-op at least in some builds.
>  - LLVMLinkInJIT() was executed regardless of weather MCJIT is to be
> used or not.
>
> Resolve that my promoting USE_MCJIT to be static bool, always. Make sure
> it's honoured and the correct LLVMLinkIn{MC,}JIT() function is called
> only as needed.
>
> Fixes: cf4105740f0 "gallivm: Make MCJIT a runtime option."
> Cc: Zhen Wu <wuzhen at jidemail.com>
> Cc: Jose Fonseca <jfonseca at vmware.com>
> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
> ---
> Jose, rather than jumping around like a headless chicken I've went ahead
> and fixed things... or maybe I broke it ? Afaict this preserves the
> original behaviour and linking should be perfectly fine.
>
> XXX: worth dropping the ALL_CAPS from the, now, variable name ? Should
> we squash it here or as separate patch ?
>
> As an added bonus might even solve the issue Wu Zhen is hitting :-)
> ---
>  src/gallium/auxiliary/gallivm/lp_bld_init.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_init.c b/src/gallium/auxiliary/gallivm/lp_bld_init.c
> index d1b2369f34..9a77c87ae4 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_init.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_init.c
> @@ -45,9 +45,9 @@
>
>  /* Only MCJIT is available as of LLVM SVN r216982 */
>  #if HAVE_LLVM >= 0x0306
> -#  define USE_MCJIT 1
> +static bool USE_MCJIT = 1;
>  #elif defined(PIPE_ARCH_PPC_64) || defined(PIPE_ARCH_S390) || defined(PIPE_ARCH_ARM) || defined(PIPE_ARCH_AARCH64)
> -#  define USE_MCJIT 1
> +static bool USE_MCJIT = 1;
>  #else
>  static bool USE_MCJIT = 0;
>  #endif
> @@ -395,11 +395,11 @@ lp_build_init(void)
>     if (gallivm_initialized)
>        return TRUE;
>
> -   LLVMLinkInMCJIT();
> -#if !defined(USE_MCJIT)
> -   USE_MCJIT = debug_get_bool_option("GALLIVM_MCJIT", 0);
> -   LLVMLinkInJIT();
> -#endif
> +   USE_MCJIT = debug_get_bool_option("GALLIVM_MCJIT", USE_MCJIT);
> +   if (USE_MCJIT)
> +      LLVMLinkInMCJIT();
> +   else
> +      LLVMLinkInJIT();
>
>  #ifdef DEBUG
>     gallivm_debug = debug_get_option_gallivm_debug();
>