[Mesa-dev] [PATCH 4/6] configure.ac: Set and use HAVE_GALLIUM_LLVM define

[Emil Velikov]

On 23 January 2017 at 19:39, Tobias Droste <tdroste at gmx.de> wrote:
> Am Montag, 23. Januar 2017, 11:53:18 CET schrieb Jose Fonseca:
>> On 20/01/17 02:48, Emil Velikov wrote:
>> > On 19 January 2017 at 19:26, Tobias Droste <tdroste at gmx.de> wrote:
>> >> Am Mittwoch, 18. Januar 2017, 18:45:04 CET schrieb Emil Velikov:
>> >>> On 18 January 2017 at 18:12, Jose Fonseca <jfonseca at vmware.com> wrote:
>> >>>>>> In order to untangle things we want to have a distinction between the
>> >>>>>> gallium (gallivm afaict) and other users - RADV presently.
>> >>>>>> So how about we update the RADV instances and ensure that the we set
>> >>>>>> the HAVE_{RADV,}_LLVM lot appropriately. Latter will be picky but
>> >>>>>> overall things should work w/o annoyances that HAVE_GALLIUM_LLVM
>> >>>>>> brings ?
>> >>>>
>> >>>> I honestly don't even understand why we'd want to build parts of the
>> >>>> tree
>> >>>> with LLVM while hiding LLVM from other components.  We can't we just
>> >>>> build
>> >>>> everything with LLVM and avoid this combinatorial explosion of wierd
>> >>>> options that are nothing more than yet another way the build can
>> >>>> break!!?
>> >>>
>> >>> Sadly the combinatoric explosion has been there for a while. Based on
>> >>> how well my previous attempts to resolve similar issues (see the
>> >>> "platforms" topic) I doubt we'll even get to fix that.
>> >>>
>> >>>> But if a separate option is truly necessary, have the newcomer pick a
>> >>>> different name, or something.
>> >>>
>> >>> That's pretty much what I suggested above. Tobias can you please give it
>> >>> a
>> >>> try ?
>> >>
>> >> I would rather "fix" the other build systems. (As in just define
>> >> HAVE_GALLIUM_LLVM if HAVE_LLVM is defined).
>> >>
>> >> I think there is still a misunderstanding on Joses side on what this
>> >> really
>> >> means. No file in gallivm or llvmpipe will be touched. It's really just
>> >> auxilliary/draw and there it's exactly 8 lines that will change.
>> >>
>> >> That's it.
>> >>
>> >> I really fail to see how this will break everything that is being worked
>> >> on
>> >> and cause merge conflicts everywhere.
>> >>
>> >> If you still want the other way, I can do that to, but this will of
>> >> course
>> >> need the same fix in the other build system or we have the same situation
>> >> we have now, but with other drivers.
>> >
>> > Afaict one point is that the use of HAVE_GALLIUM_LLVM vs HAVE_LLVM is
>> > too subtle. Let's not forget that barring the WIP(?) branches, VMWare
>> > has closed source components. Guess how much fun it will be as
>> > suddenly things fail to build/work properly as they re-sync the code
>> > base. No idea how likely the latter is, but considering Jose (and a
>> > few other VMWare guys) wrote sizeable hunk of that code (and Mesa as a
>> > whole) I'd go with his instinct.
>> >
>> > Emil
>>
>> The HAVE_LLVM->HAVE_GALLIUM_LLVM rename is indeed not as invasive as I
>> thought.
>>
>> But I still don't understand why HAVE_LLVM->HAVE_GALLIUM_LLVM is
>> necessary in draw and not on gallivm/llvmpipe.
>>
>> People want to build draw with LLVM support, but without
>> gallivm/llvmpipe? That's impossible.
>>
>> Or is this because the draw files are the only .c files that are
>> compiled even when HAVE_LLVM is undefined, so these are the only ones
>> that get to receive the renaming treatment?  That's crazy confusing.
>> There's no away I can accept that.
>>
>
> The draw files are used by softpipe (and maybe other gallium drivers, haven't
> checked that) and there HAVE_LLVM should not be defined. If it's not,
> everything is fine. But with new non gallium drivers using LLVM and causing
> HAVE_LLVM to bedefined, softpipe is broken in some cases. See below.
>
>>
>> Let me make this crystal clear to avoid making this discussion even more
>> protracted: I will not accept any HAVE_LLVM change in
>> draw/gallivm/llvmpipe .C/.H source code.  Period.
>>
>
> I'm _not_ changing gallivm and llvmpipe. Draw is not only used by llvmpipe and
> I still think I have very good arguments for the change. See again below.
>
> I understand, I'm the unknown new guy and you did a lot of work in this code.
> But I'm not getting paid for this and I don't have to do this. I want to help,
> but I also want to understand why I can't do something. With reasons other
> than "I say so, and I don't want to hear any reasons against it". I hope you
> understand that.
>
>>
>> HAVE_LLVM used to mean, "whole Mesa being built with LLVM".  Now people
>> want to build something (no idea what yet to be honest) with LLVM, but
>> not build draw/gallivm/llvmpipe.
>>
>> If you want to build some other component with LLVM but not
>> draw/gallivm/llvmpipe, then add a new HAVE_LLVM_FOR_FOOBAR define and
>> use it where you need it.
>
> The real problem is softpipe. Softpipe uses draw but (obviously) can't use
> LLVM.
>
> Right now one could build radv (uses LLVM) and pass --disable-gallium-llvm to
> the build system to get softpipe built.
>
> But due to radv "HAVE_LLVM" (which is used as a version check everywhere
> else!) is defined and the draw code gets build using gallivm (since HAVE_LLVM
> is defined). So the resulting softpipe will actually use LLVM even though you
> deselected LLVM for gallium.
>
> That's how it is right now!
>
> The draw code is the only code that has optional LLVM support and also the
> only code that uses HAVE_LLVM without any version check just to see if it
> should use LLVM or not. That's why this is the obvious and easy to change
> component.
>
Trying to reword things:
 - optional build against LLVM has been around since day 1, afaict.
 - we have code (galliuvm, llvmpipe, etc.) which is built only when
llvm is present and other (draw) which has conditional ifdef HAVE_LLVM
hunks
 - latter of ^^ is required since we want to have softpipe only (i.e.
w/o llvmpipe) even though it uses draw
 - [side note] other drivers (such as r300 and nouveau, nv30 in
particular) also use draw
 - the macro name was changed only in draw, since everything else is
build _only_ if we have LLVM.
Outside of draw, HAVE_LLVM _is_ a misnomer since we use it it as a
version check, rather than presence.
 - some parts (radv, others?) do not use draw therefore the configure
toggle and thus the definition of HAVE_LLVM are off.
HAVE_LLVM has the meaning of USE_LLVM_FOR_DRAW within draw.

> I'm open for a rename. USE_LLVM_FOR_DRAW?
> I would still argue that this is the right place to do the change!
>
Fwiw I agree that renaming it in gallium/draw would be better. There's
a couple of things though:
- it should be pretty clear (or otherwise one can get the whole thing
within 2 mins or so) what this new define is a why/where would one
need it.
Perhaps adding a few comments throughout and/or a small README ?
- one has to get the devs that work on that code (git log) behind this.
With the above sorted this shouldn't be too hard, imho.

> But having said all that:
> If we're all okay to disallow a mixed LLVM/no-LLVM build, I'm fine with that
> too. In that case there's no renaming to be done.
> Right now that sould only break configurations where radv is somehow involved
> as all the other LLVM users are gallium drivers as far as I can tell.
>
> Emil? :-)
>
You're suggesting that we force require --enable-gallium-llvm when
building RADV ? Not too big of a fan I'm afraid.
If possible I'd leave that as the final option.

Thanks
Emil