[Mesa-dev] [PATCH 4/4] OpenSWR build changes

Mon Feb 22 22:15:27 UTC 2016

On 22 February 2016 at 20:39, Rowley, Timothy O
<timothy.o.rowley at intel.com> wrote:
> Thanks for taking the time to dig into this patch.  Figured I’d address a few of the comments now, and work on all your points for the next revision.
>
>> On Feb 22, 2016, at 12:53 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>
>> On 18 February 2016 at 01:53, Tim Rowley <timothy.o.rowley at intel.com> wrote:
>>
>> Don't be shy to mention something in the commit message - which of the
>> tree targets has been tested. Do they all build/work on linux,
>> windows, BSD, other. Pretty much anything that you believe it's
>> important. You might also include something that is known to not build
>> properly (be that explicitly disabled atm or not).
>
> Will do.  Preview: we have tested only on Linux (CentOS, Ubuntu, and SuSE based) and have started working towards working on Windows, but are aware that it is not currently working (thus disabled).
>
Anything like the above will be great to note in the commit msg.

>>> diff --git a/scons/custom.py b/scons/custom.py
>>> index 043793b..a5a3410 100644
>>> --- a/scons/custom.py
>>> +++ b/scons/custom.py
>>> @@ -132,7 +132,7 @@ def code_generate(env, script, target, source, command):
>>>     script_src = env.File(script).srcnode()
>>>
>>>     # This command creates generated code *in the build directory*.
>>> -    command = command.replace('$SCRIPT', script_src.path)
>>> +    command = command.replace('$SCRIPT', script_src.rstr())
>> Looks like unrelated change which I'd split out in separate patch.
>
> This change was needed to get the scons variant_dir approach of building a directory twice to work.
>
Sure thing. Pop it in an earlier commit and say something like "it
does X as opposed to the current Y. we'll require the former with the
follow-up openswr build infrastructure". Sorry to bother you with
this, but my scons-foo is not ideal so I'm making sure that things are
more digestable - be that for review or, if really needed, for revert.

>>> diff --git a/scons/llvm.py b/scons/llvm.py
>>> index 1fc8a3f..30742ff 100644
>>> --- a/scons/llvm.py
>>> +++ b/scons/llvm.py
>>
>>> @@ -128,7 +129,8 @@ def generate(env):
>>>                 'LLVMX86Info', 'LLVMX86AsmPrinter', 'LLVMX86Utils',
>>>                 'LLVMMCJIT', 'LLVMTarget', 'LLVMExecutionEngine',
>>>                 'LLVMRuntimeDyld', 'LLVMObject', 'LLVMMCParser',
>>> -                'LLVMBitReader', 'LLVMMC', 'LLVMCore', 'LLVMSupport'
>>> +                'LLVMBitReader', 'LLVMMC', 'LLVMCore', 'LLVMSupport',
>>> +                'LLVMIRReader', 'LLVMAsmParser', 'LLVMX86AsmParser'
>> I'm thinking that we'll need these (and perhaps the irreader below)
>> for the automake/conf build as well. Set --disable-llvm-shared-libs
>> and watch things burn ;-)
>
> Those changes were done for our windows port attempt.  I have noticed the reliance on the unified llvm shared lib on linux.
>
With the configure option mentioned above, one toggles between the
monolitic shared llvm library and the multiple static ones. I'd
imagine that some of those will be needed as well, it doesn't look
right if all of these are Windows specific ?

>>> --- a/src/gallium/SConscript
>>> +++ b/src/gallium/SConscript
>>> @@ -20,6 +20,9 @@ SConscript([
>>>     'drivers/trace/SConscript',
>>> ])
>>>
>>> +if env['platform'] != 'windows':
>>> +   SConscript('drivers/swr/SConscript')
>>> +
>> I take it that you're planning to change this at some point in the
>> future ? Some of the scon files below explicitly check against msvc.
>
> Yes, this was temporary to disable a configuration which was known broken.
>
Please add a FINISHME or any other form of comment. Just in case.

>>> --- /dev/null
>>> +++ b/src/gallium/drivers/swr/.clang-format
>> Not a buildsystem file/change, but it's fine ;-)
>
> Yes, slipped in my splitting of the patchset and should have gone with the openswr driver source.
>
>>> diff --git a/src/gallium/drivers/swr/Makefile.am b/src/gallium/drivers/swr/Makefile.am
>>> new file mode 100644
>>> index 0000000..f3a4321
>>> --- /dev/null
>>> +++ b/src/gallium/drivers/swr/Makefile.am
>>> @@ -0,0 +1,37 @@
>>> +# Copyright (C) 2015 Intel Corporation.   All Rights Reserved.
>>> +#
>>> +# Permission is hereby granted, free of charge, to any person obtaining a
>>> +# copy of this software and associated documentation files (the "Software"),
>>> +# to deal in the Software without restriction, including without limitation
>>> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> +# and/or sell copies of the Software, and to permit persons to whom the
>>> +# Software is furnished to do so, subject to the following conditions:
>>> +#
>>> +# The above copyright notice and this permission notice (including the next
>>> +# paragraph) shall be included in all copies or substantial portions of the
>>> +# Software.
>>> +#
>>> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>>> +# IN THE SOFTWARE.
>>> +
>>> +AUTOMAKE_OPTIONS = subdir-objects
>>> +
>> We don't need this. It's already enabled globally.
>
> That was most likely copied from another driver, but it looks like we’re the holdouts now.
>
>>> diff --git a/src/gallium/drivers/swr/Makefile.sources b/src/gallium/drivers/swr/Makefile.sources
>>> new file mode 100644
>>> index 0000000..4317a85
>>> --- /dev/null
>>> +++ b/src/gallium/drivers/swr/Makefile.sources
>>> @@ -0,0 +1,23 @@
>>> +# Copyright (C) 2015 Intel Corporation.   All Rights Reserved.
>>> +#
>>> +# Permission is hereby granted, free of charge, to any person obtaining a
>>> +# copy of this software and associated documentation files (the "Software"),
>>> +# to deal in the Software without restriction, including without limitation
>>> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> +# and/or sell copies of the Software, and to permit persons to whom the
>>> +# Software is furnished to do so, subject to the following conditions:
>>> +#
>>> +# The above copyright notice and this permission notice (including the next
>>> +# paragraph) shall be included in all copies or substantial portions of the
>>> +# Software.
>>> +#
>>> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>>> +# IN THE SOFTWARE.
>>> +
>>> +CXX_SOURCES := \
>>> +       swr_loader.cpp
>>
>> I'd fold this and the Makefile.sources-arch into a single file.
>
> automake didn’t like it when there were defines in the Makefile.sources not being used by the including file.
>
You can provide different names - for example the above can be
LOADER_SOURCES or pretty much anything you like really.

>>> diff --git a/src/gallium/drivers/swr/SConscript-arch b/src/gallium/drivers/swr/SConscript-arch
>>> new file mode 100644
>>> index 0000000..b20a1f8
>>> --- /dev/null
>>> +++ b/src/gallium/drivers/swr/SConscript-arch
>>> @@ -0,0 +1,117 @@
>>
>>> +env.Prepend(LIBS = [ mesautil, mesa, gallium ])
>>> +
>> Please drop this. Linking should happen in the final stage (gallium/targets).
>
> Since we’re building standalone libswrAVX.so and libswrAVX2.so libraries which are loaded from a library with default hidden visibility, unfortunately all the dependent symbols need to be linked in this “final” stage (env.SharedLibrary() target).
>
Ahh yes. For some reason I've thinking that these incorporate the
Roland (and my unspoken) suggestion -> fold the loader and backends
into a single library.

>>> diff --git a/src/gallium/drivers/swr/avx/Makefile.am b/src/gallium/drivers/swr/avx/Makefile.am
>>> new file mode 100644
>>> index 0000000..c09fb5a
>>> --- /dev/null
>>> +++ b/src/gallium/drivers/swr/avx/Makefile.am
>>> @@ -0,0 +1,112 @@
>>> +include ../Makefile.sources-arch
>>> +include $(top_srcdir)/src/gallium/Automake.inc
>>> +
>>> +VPATH = $(srcdir) $(srcdir)/..
>>> +
>> Yuk why do we need this ?
>
> This was in lieu of adding another “../“ to all the Makefile.sources - an approach I took originally but didn’t work for reasons I forget at the moment.
>>
>>> +libswrAVX_la_LIBADD = \
>>> +       $(top_builddir)/src/gallium/auxiliary/libgallium.la \
>>> +       $(top_builddir)/src/mesa/libmesagallium.la
>>> +
>> Keep these in the final link stage.
>
> Same comment as the SCons build system - this is a final link stage.
>
>>> +include $(top_srcdir)/install-gallium-links.mk
>>> +
>> Those are used for compatibility reasons with some developers'
>> workflow. I doubt you want these.
>
> Actually I did want those. :)  Including this copied/created the links in lib/gallium.
>
Boooo /me looks for some rotten fruit to throw. But seriously: as you
wish, I've assumed it was a copy/pasta from another file.

>>> --- /dev/null
>>> +++ b/src/gallium/drivers/swr/avx2/Makefile.am
>>
>> The avx comments apply here as well. Me is feeling like a sad panda
>> that we cannot do the clever scons trick in here somehow.
>
> Yes, I tried creating a common Makefile.am-sources, but automake didn’t want to do the appropriate variable substitution.
>
The only thing that I can think of is having swr/Automake.inc
(interesting name that we're stuck using) file, which is include(de)
by avx{,2}/Makefile.am. The former contains all the common bits with
the latter alike:

--
include ../Makefile.sources-arch
include ../Automake.inc
include $(top_srcdir)/src/gallium/Automake.inc

AM_CXXFLAGS = \
       -I$(srcdir)/../rasterizer \
       -I$(srcdir)/../rasterizer/core \
       -I$(builddir)/rasterizer/jitter \
       -I$(srcdir)/../rasterizer/jitter \
       -I$(builddir)/rasterizer/scripts \
       $(GALLIUM_DRIVER_CFLAGS) \
       -std=c++11 \
       -march=core-avx-i \
       -DKNOB_ARCH=KNOB_ARCH_AVX \
       $(LLVM_CFLAGS)

lib_LTLIBRARIES = libswrAVX.la

libswrAVX_la_SOURCES = $(ALL_SOURCES)

libswrAVX_la_LIBADD = $(ALL_LIBDEPS)
--

It's not as brief as the scons one but it ought to work ;-)

Are you guys using in tree or out of tree builds for autotools ? If
one of them doesn't work (it's fine we can sort them later on) please
mention it in the commit message.

Thanks
Emil