On 22 June 2016 at 21:01, Rowley, Timothy O <timothy.o.row...@intel.com> wrote:
>
>> 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.
>
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
