[Mesa-dev] [PATCH 1/3] gallium/scons: OpenSWR Windows support

[Kyriazis, George]

Thank you for the review.  Comments inline.

> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
> Sent: Tuesday, November 8, 2016 7:52 AM
> To: Kyriazis, George <george.kyriazis at intel.com>; Jose Fonseca
> <jfonseca at vmware.com>
> Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [PATCH 1/3] gallium/scons: OpenSWR Windows
> support
> 
> Hi George,
> 
> For Scons changes please keep Jose Fonseca in the loop.
> 
> On 7 November 2016 at 22:32, George Kyriazis <george.kyriazis at intel.com>
> wrote:
> > - Added code to create screen and handle swaps in libgl_gdi.c
> > - Added call to swr SConscript
> > - included llvm 3.9 support for scons (windows swr only support 3.9 and
> >   later)
> If that's the case building SWR with earlier one should error out ?
> Then again, here you reference gallium/drivers/swr/
> 
Yes, SWR will only work on windows for 3.9 and above.

> > - include -DHAVE_SWR to subdirs that need it
> >
> As the above indicates here you have multiple independent changes.
> Please do _not_ mix those into a single patch.
> 
I'll resend v2 of the patches with a different breakdown.

Additional comments on your review of patch 3/3.

Thanks,

George

> 
> > To buils SWR on windows, use "scons swr libgl-gdi"
> > ---
> >  scons/llvm.py                             | 21 +++++++++++++++++++--
> >  src/gallium/SConscript                    |  1 +
> >  src/gallium/targets/libgl-gdi/SConscript  |  4 ++++
> > src/gallium/targets/libgl-gdi/libgl_gdi.c | 28
> > +++++++++++++++++++++++-----  src/gallium/targets/libgl-xlib/SConscript
> |  4 ++++
> >  src/gallium/targets/osmesa/SConscript     |  4 ++++
> >  6 files changed, 55 insertions(+), 7 deletions(-)
> >
> > diff --git a/scons/llvm.py b/scons/llvm.py index 1fc8a3f..977e47a
> > 100644
> > --- a/scons/llvm.py
> > +++ b/scons/llvm.py
> > @@ -106,7 +106,24 @@ def generate(env):
> >          ])
> >          env.Prepend(LIBPATH = [os.path.join(llvm_dir, 'lib')])
> >          # LIBS should match the output of `llvm-config --libs engine mcjit
> bitwriter x86asmprinter`
> > -        if llvm_version >= distutils.version.LooseVersion('3.7'):
> > +        if llvm_version >= distutils.version.LooseVersion('3.9'):
> > +            env.Prepend(LIBS = [
> > +                'LLVMX86Disassembler', 'LLVMX86AsmParser',
> > +                'LLVMX86CodeGen', 'LLVMSelectionDAG', 'LLVMAsmPrinter',
> > +                'LLVMDebugInfoCodeView', 'LLVMCodeGen',
> > +                'LLVMScalarOpts', 'LLVMInstCombine',
> > +                'LLVMInstrumentation', 'LLVMTransformUtils',
> > +                'LLVMBitWriter', 'LLVMX86Desc',
> > +                'LLVMMCDisassembler', 'LLVMX86Info',
> > +                'LLVMX86AsmPrinter', 'LLVMX86Utils',
> > +                'LLVMMCJIT', 'LLVMExecutionEngine', 'LLVMTarget',
> > +                'LLVMAnalysis', 'LLVMProfileData',
> > +                'LLVMRuntimeDyld', 'LLVMObject', 'LLVMMCParser',
> > +                'LLVMBitReader', 'LLVMMC', 'LLVMCore',
> > +                'LLVMSupport',
> > +                'LLVMIRReader', 'LLVMASMParser'
> > +            ])
> LLVM 3.9 support. cc: mesa-stable (if Jose/Brian are up for it).
> 
> > +        elif llvm_version >= distutils.version.LooseVersion('3.7'):
> >              env.Prepend(LIBS = [
> >                  'LLVMBitWriter', 'LLVMX86Disassembler', 'LLVMX86AsmParser',
> >                  'LLVMX86CodeGen', 'LLVMSelectionDAG',
> > 'LLVMAsmPrinter', @@ -203,7 +220,7 @@ def generate(env):
> >              if '-fno-rtti' in cxxflags:
> >                  env.Append(CXXFLAGS = ['-fno-rtti'])
> >
> > -            components = ['engine', 'mcjit', 'bitwriter', 'x86asmprinter',
> 'mcdisassembler']
> > +            components = ['engine', 'mcjit', 'bitwriter',
> > + 'x86asmprinter', 'mcdisassembler', 'irreader']
> Standalone bugfix. Cc: mesa-stable ?
> 
> 
> > +++ b/src/gallium/SConscript
> 
> > +    'drivers/swr/SConscript',
> This file is only introduced with 3/3. Which means that you've added scons
> support which is broken - please don't do that.
> 
> 
> > +++ b/src/gallium/targets/libgl-gdi/SConscript
> > +++ b/src/gallium/targets/libgl-gdi/libgl_gdi.c
> > +++ b/src/gallium/targets/libgl-xlib/SConscript
> > +++ b/src/gallium/targets/osmesa/SConscript
> 
> Couple of ideas how to split these. Or anything else that comes to mind on
> your end.
> 
> A)
> Patch 1
> src/gallium/SConscript
> src/gallium/targets/libgl-gdi/SConscript
> src/gallium/targets/libgl-gdi/libgl_gdi.c
> Patch 2
> src/gallium/targets/libgl-xlib/SConscript
> src/gallium/targets/osmesa/SConscript
> 
> B)
> Patch 1
> src/gallium/targets/libgl-gdi/libgl_gdi.c
> Patch 2
> src/gallium/SConscript
> src/gallium/targets/libgl-gdi/SConscript
> src/gallium/targets/libgl-xlib/SConscript
> src/gallium/targets/osmesa/SConscript
> 
> 
> Thanks
> Emil