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

[Rowley, Timothy O]

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

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

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

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

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

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

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

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