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

[Emil Velikov]

On 16 January 2017 at 14:44, Jose Fonseca <jfonseca at vmware.com> wrote:
> 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.
>
A nice step forward. s/USE_MCJIT/HAVE_MCJIT/ might also be nice, but I
won't nag any more :-)
Fwiw the patch is
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

>> 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.
>
Depending on how much you (in general) recall about LLVMLinkIn* each
approach is confusing. Pretty much any comment would be beneficial.

>>
>> How does that sound ?
>> Emil
>
>
> This patch does nothing for Android, but it should now be trivial for Zhen
> Wu to rebase his patch.
>
Agreed.

Thanks
Emil