> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> Sent: Tuesday, November 8, 2016 10:54 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 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.
>
That explains it. Ok, I'll take care of it.
> >> > +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.
>
Can I take care of it on a follow-on check-in? Ie. check-in as-is for now?
> >> > +# 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.
>
Ok, will do.
Thanks,
George
> Thanks
> Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
