> On Jun 22, 2016, at 2:27 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
>
> On 22 June 2016 at 20:19, Rowley, Timothy O <timothy.o.row...@intel.com>
> wrote:
>>
>>> On Jun 22, 2016, at 1:52 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
>>>
>>> On 20 June 2016 at 22:36, Tim Rowley <timothy.o.row...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
