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

[Jose Fonseca]

On 16/01/17 13:46, Emil Velikov wrote:
> On 14 January 2017 at 08:46, Jose Fonseca <jfonseca at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: use_mcjit.patch
Type: text/x-patch
Size: 3267 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170116/47c0d4f1/attachment-0001.bin>