On 8 November 2016 at 15:48, Kyriazis, George <george.kyria...@intel.com> wrote:
> Comments inline..
>
>> -----Original Message-----
>> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
>> Sent: Tuesday, November 8, 2016 8:25 AM
>> To: Kyriazis, George <george.kyria...@intel.com>
>> Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org>
>> Subject: Re: [Mesa-dev] [PATCH 3/3] swr: Support windows builds
>>
>> On 7 November 2016 at 22:32, George Kyriazis <george.kyria...@intel.com>
>> wrote:
>> > - Added SConscript files
>> > - better handling of NOMINMAX for <windows.h> inclusion
>> > - Reorder header files in swr_context.cpp to handle NOMINMAX better,
>> since
>> > mesa header files include windows.h before we get a chance to #define
>> > NOMINMAX
>> > - cleaner support for .dll and .so prefix/suffix across OSes
>> > - added PUBLIC for some protos
>> > - added swr_gdi_swap() which is call from libgl_gdi.c
>> > ---
>> > src/gallium/drivers/swr/Makefile.am | 8 ++
>> > src/gallium/drivers/swr/SConscript | 46 +++++++
>> > src/gallium/drivers/swr/SConscript-arch | 175
>> +++++++++++++++++++++++++
>> > src/gallium/drivers/swr/rasterizer/common/os.h | 5 +-
>> > src/gallium/drivers/swr/swr_context.cpp | 16 +--
>> > src/gallium/drivers/swr/swr_context.h | 2 +
>> > src/gallium/drivers/swr/swr_loader.cpp | 37 +++++-
>> > src/gallium/drivers/swr/swr_public.h | 11 +-
>> > src/gallium/drivers/swr/swr_screen.cpp | 25 +---
>> > 9 files changed, 291 insertions(+), 34 deletions(-) create mode
>> > 100644 src/gallium/drivers/swr/SConscript
>> > create mode 100644 src/gallium/drivers/swr/SConscript-arch
>> >
>> Similar to 1/3 this patch does too many things. Please _don't_ do that.
>>
>> Some ideas based on the above:
>> - source code fixes - one or multiple patches, depending on details.
>> - automake fixes - ^^
>> - introduce scons build (+ the EXTRA_DIST hunk)
>>
> As stated in review of patch 1/3, I will send v2 of patches with different
> breakdown.
>
>
>> Some misc comments below.
>>
>>
>> > +++ b/src/gallium/drivers/swr/SConscript
>> > @@ -0,0 +1,46 @@
>> > +Import('*')
>> > +
>> > +from sys import executable as python_cmd import distutils.version
>> Seems unused. Maybe it was aimed for the llvm 3.9 requirement/check
>> mentioned in 1/3 ?
>>
> Scons build fails without the Import('*'), because env is undefined:
>
> NameError: name 'env' is not defined:
>
The "unused" comment was meant for the "import distutils.version"
line. Which seemingly got manged somewhere along the way.
>> > +import os.path
>> > +
>> > +if not 'swr' in COMMAND_LINE_TARGETS:
>> > + Return()
>> > +
>> > +if not env['llvm']:
>> > + print 'warning: LLVM disabled: not building swr'
>> > + Return()
>> > +
>> > +env.MSVC2013Compat()
>> > +
>>
>> > +swr_arch = 'avx'
>> > +VariantDir('avx', '.', duplicate=0)
>> > +SConscript('avx/SConscript-arch', exports='swr_arch')
>> > +
>> > +swr_arch = 'avx2'
>> > +VariantDir('avx2', '.', duplicate=0)
>> > +SConscript('avx2/SConscript-arch', exports='swr_arch')
>> > +
>> Afaict one can just fold the SConscript-arch here. Thus one won't need to
>> bother with the above nor the Depends hunk below.
>> Additionally with current approach one is generating [the] identical source
>> files twice. Far from ideal...
>>
> The AVX and AVX2 builds build differently (with different compiler flags).
> At runtime, we load the appropriate dll, based on the underlying
> architecture. We do the same thing on the linux build. Also, since
> duplicate=0, source is not duplicated. Yes, generated files are generated
> twice, however currently SConscript is just a shell around SConscript-arch;
> all the logic that generates the files and source lists is in
> SConscript-arch. By moving the auto generation to SConscript will generate
> only one copy of the gen files, however it splits the build logic into two
> files, which is more messy. I can certainly move the generation code in
> SConscript, however, I think that it's cleaner to strive for source code
> cleanliness, as opposed to generate code cleanliness.
"By moving the auto generation to SConscript ..., however it splits
the build logic into two files..." did you mean "one file" here ?
I'm proposing to fold the two SConscripts, which effectively moves the
build logic into _one_ file :-)
Scons was never my thing, so I'm failing to see the "source code
cleanliness" that you're thinking about :-(
The following isn't that messy is it ?
build loader
generate sources
build avx - (uses above sources + avx compile flags)
build avx2 - (uses above sources + avx2 compile flags)
This is now I folded/cleaned up the autoconf build with commit
bb949e262cb5c4fffe991debc605447e15322666. A similar solution here
would be great/possible.
>> > +# remove headers, as scons thinks they are static objects for the .so
>> > +source = [x for x in source if not x.endswith(tuple(['.h','.hpp']))]
>> > +
>> Should be handled already. Otherwise please do so in scons/* Quick grep
>> suggests scons/custom.py
>>
> ParseSourceList() will filter out .h files, however it won't filter out .hpp
> files. Are you saying add the .hpp filter in custom.py?
>
Yes, that's correct. Just expand the existing infra to manage .hpp
alongside .h files.
Thanks
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
