[Mesa-dev] [PATCH v2 05/14] swr: [rasterizer jitter] cleanup supporting different llvm versions

[Emil Velikov]

On 22 June 2016 at 21:01, Rowley, Timothy O <timothy.o.rowley at intel.com> wrote:
>
>> On Jun 22, 2016, at 2:27 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>
>> On 22 June 2016 at 20:19, Rowley, Timothy O <timothy.o.rowley at intel.com> wrote:
>>>
>>>> On Jun 22, 2016, at 1:52 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>
>>>> On 20 June 2016 at 22:36, Tim Rowley <timothy.o.rowley at intel.com> wrote:
>>>>> ---
>>>>> .../drivers/swr/rasterizer/jitter/JitManager.cpp   |  9 +++++--
>>>>> .../drivers/swr/rasterizer/jitter/JitManager.h     |  7 ++++-
>>>>> .../drivers/swr/rasterizer/jitter/blend_jit.cpp    |  8 +-----
>>>>> .../drivers/swr/rasterizer/jitter/builder_misc.cpp | 31 +++++++++++++++++++---
>>>>> .../drivers/swr/rasterizer/jitter/builder_misc.h   |  6 +++++
>>>>> .../drivers/swr/rasterizer/jitter/fetch_jit.cpp    | 15 ++---------
>>>>> .../jitter/scripts/gen_llvm_ir_macros.py           | 24 ++++++++++++++++-
>>>>> .../swr/rasterizer/jitter/streamout_jit.cpp        |  7 +----
>>>>> 8 files changed, 73 insertions(+), 34 deletions(-)
>>>>>
>>>>> diff --git a/src/gallium/drivers/swr/rasterizer/jitter/JitManager.cpp b/src/gallium/drivers/swr/rasterizer/jitter/JitManager.cpp
>>>>> index 4bbd9ad..6e00a70 100644
>>>>> --- a/src/gallium/drivers/swr/rasterizer/jitter/JitManager.cpp
>>>>> +++ b/src/gallium/drivers/swr/rasterizer/jitter/JitManager.cpp
>>>>> @@ -35,11 +35,13 @@
>>>>> #include "JitManager.h"
>>>>> #include "fetch_jit.h"
>>>>>
>>>>> +#pragma push_macro("DEBUG")
>>>>> +#undef DEBUG
>>>>> +
>>>>> #if defined(_WIN32)
>>>>> #include "llvm/ADT/Triple.h"
>>>>> #endif
>>>>> #include "llvm/IR/Function.h"
>>>>> -#include "llvm/Support/DynamicLibrary.h"
>>>>>
>>>>> #include "llvm/Support/MemoryBuffer.h"
>>>>> #include "llvm/Support/SourceMgr.h"
>>>>> @@ -53,6 +55,8 @@
>>>>> #include "llvm/ExecutionEngine/JITEventListener.h"
>>>>> #endif
>>>>>
>>>>> +#pragma pop_macro("DEBUG")
>>>>> +
>>>> I'm afraid that these still are still off - they should be wrapped in
>>>> "if HAVE_LLVM >= 0x0307 ... endif". Plus the ones in JitManager.h
>>>> really want a similar treatment.
>>>
>>> Any reason to avoid the push/pop on older LLVM?  Saves things from becoming too messy with preprocessor directives.
>>>
>> Because those are used by gallium (and mesa). If you undefine it here,
>> then somewhere down the chain of includes you'll end up in headers
>> that use it and things will go meh.
>
> I think I’m missing something obvious - the push/undef/pop sequence surrounds just the llvm includes in JitManager.{h,cpp}, and at the end of pop_macro the DEBUG macro will be back to what it was originally defined as.  The reason for adding them is to isolate the llvm usage.
>
It's a bit fragile, as there's nothing stopping others (yourself X
months down the line) from moving a swr/gallium header between the
push and pop. But at the end of the day I won't be debugging (if it
breaks) or keeping track, so don't mind me.

>>
>> a>> Mildly related bugs/cleanups:
>>>> - There's a few cases of _DEBUG which should (?) be replaced with ifndef NDEBUG
>>>
>>> Ok, I can address this in another patch.
>>>
>> IMHO it's worth sorting both identical issues (and checking for other
>> offenders) in one patch. Be that here, or as follow on it's up-to you.
>
> _DEBUG usage spills into rasterizer/{common,core} which is why I was thinking of addressing it in a different commit rather than this one which concerns itself just with the jitter directory.
>
Apologies, got the line ordering wrong - my suggestion does not apply here.

>>>> - swr uses both mesa and LLVM provided version macros. Please stick to one.
>>>> If the latter is reliable (available all the way to min. supported
>>>> LLVM version) and can be used in both C and C++ sources I'm inclined
>>>> to just use it everywhere in mesa and drop out local macros…
>>>
>>> Are you referring to the HAVE_LLVM macro?  I can remove the conditional definition of this from swr (since Mesa provides the definition).
>>>
>> Mesa provides HAVE_LLVM and MESA_LLVM_VERSION_PATCH while LLVM does
>> LLVM_VERSION_{MAJOR,MINOR,PATCH}. Personally I'm in faviour of the
>> latter (considering the 'reliable' note above), but using one of them
>> (regardless which) is what you want imho.
>
> Conditional code for different llvm versions is much easier with the HAVE_LLVM style combined major/minor rather than using LLVM_VERSION_* (which if not done properly will break if they ever switch to llvm-4.x).
>
I hope they don't break that one ... there'll be dozens of projects
that'll be busted :-)

-Emil