On 17/01/17 13:26, Emil Velikov wrote:
> On 16 January 2017 at 14:44, Jose Fonseca <jfons...@vmware.com> wrote:
>> 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.
>>
> 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.veli...@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
>
I added an additional comment re. LLVMLinkIn* and pushed. Thanks.
Jose
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
