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

[Emil Velikov]

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.

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.

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

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