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.veli...@gmail.com> wrote:
>
> On 18 February 2016 at 01:53, Tim Rowley <timothy.o.row...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
