On 16/01/17 13:46, Emil Velikov wrote:
On 14 January 2017 at 08:46, Jose Fonseca <[email protected]> wrote: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.I might be the only one here, but having FOO (USE_MCJIT in this case) as define or variable depending on $heuristics reads a bit iffy.
Good point. I'm attaching a patch that addresses this.
That aside - if I understood you correctly:
- One of LLVMLinkIn{MC,}JIT might be missing on some versions of LLVM.
In that case having a guard called "USE" sounds like a misnomer.
Providing a static inline as needed might be cleaner ?
I didn't use an inline but I separated the define and variable more clearly.
- If LLVMLinkInJIT/LLVMLinkInMCJIT are solely for linking purposes, it will be better (imho) to add a small comment and still have them honour the user selection. With the latter, since we don't want things to explode as older/newer LLVM adds specific code in said functions.
That's bordering paranoia. The semantics of LLVMLinkIn* are clear. Yes, upstream can break them as they can break the semantics of any other function we use. It makes no sense to trust upstream to keep backwards compatability for some functions and not others.
And I still think that's guarding the LLVMLinkIn in runtime checks is more evil (because it's misleading) than good.
How does that sound ? Emil
This patch does nothing for Android, but it should now be trivial for Zhen Wu to rebase his patch.
Jose
commit 411eb361e284ec26f875daa84de4a181dcea9288 Author: Jose Fonseca <[email protected]> Date: Mon Jan 16 14:19:36 2017 +0000 gallivm: Cleanup USE_MCJIT. Split USE_MCJIT macro dual nature into a separate constant time define and a run-time variable. diff --git a/src/gallium/auxiliary/gallivm/lp_bld_init.c b/src/gallium/auxiliary/gallivm/lp_bld_init.c index d1b2369..ada823b 100644 --- a/src/gallium/auxiliary/gallivm/lp_bld_init.c +++ b/src/gallium/auxiliary/gallivm/lp_bld_init.c @@ -48,8 +48,12 @@ # define 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 +#endif + +#if defined(USE_MCJIT) +static const bool use_mcjit = USE_MCJIT; #else -static bool USE_MCJIT = 0; +static bool use_mcjit = FALSE; #endif @@ -190,7 +194,7 @@ gallivm_free_ir(struct gallivm_state *gallivm) FREE(gallivm->module_name); - if (!USE_MCJIT) { + if (!use_mcjit) { /* Don't free the TargetData, it's owned by the exec engine */ } else { if (gallivm->target) { @@ -248,7 +252,7 @@ init_gallivm_engine(struct gallivm_state *gallivm) gallivm->module, gallivm->memorymgr, (unsigned) optlevel, - USE_MCJIT, + use_mcjit, &error); if (ret) { _debug_printf("%s\n", error); @@ -257,7 +261,7 @@ init_gallivm_engine(struct gallivm_state *gallivm) } } - if (!USE_MCJIT) { + if (!use_mcjit) { gallivm->target = LLVMGetExecutionEngineTargetData(gallivm->engine); if (!gallivm->target) goto fail; @@ -336,7 +340,7 @@ init_gallivm_state(struct gallivm_state *gallivm, const char *name, * complete when MC-JIT is created. So defer the MC-JIT engine creation for * now. */ - if (!USE_MCJIT) { + if (!use_mcjit) { if (!init_gallivm_engine(gallivm)) { goto fail; } @@ -395,10 +399,16 @@ lp_build_init(void) if (gallivm_initialized) return TRUE; - LLVMLinkInMCJIT(); -#if !defined(USE_MCJIT) - USE_MCJIT = debug_get_bool_option("GALLIVM_MCJIT", 0); +#if defined(USE_MCJIT) +# if USE_MCJIT + LLVMLinkInMCJIT(); +# else + LLVMLinkInJIT(); +# endif +#else + use_mcjit = debug_get_bool_option("GALLIVM_MCJIT", FALSE); LLVMLinkInJIT(); + LLVMLinkInMCJIT(); #endif #ifdef DEBUG @@ -457,7 +467,7 @@ lp_build_init(void) util_cpu_caps.has_f16c = 0; util_cpu_caps.has_fma = 0; } - if (HAVE_LLVM < 0x0304 || !USE_MCJIT) { + if (HAVE_LLVM < 0x0304 || !use_mcjit) { /* AVX2 support has only been tested with LLVM 3.4, and it requires * MCJIT. */ util_cpu_caps.has_avx2 = 0; @@ -609,7 +619,7 @@ gallivm_compile_module(struct gallivm_state *gallivm) debug_printf("Invoke as \"llc -o - %s\"\n", filename); } - if (USE_MCJIT) { + if (use_mcjit) { assert(!gallivm->engine); if (!init_gallivm_engine(gallivm)) { assert(0);
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
