On 16/01/17 13:46, Emil Velikov wrote:
On 14 January 2017 at 08:46, Jose Fonseca <jfons...@vmware.com> 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 <jfons...@vmware.com>
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
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
