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

[Rowley, Timothy O]

> 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.

> 
> 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.

>>> - 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).

> All of ^^ are just ideas, feel free to take or leave them.
> -Emil