[Mesa-dev] [PATCH 09/70] st/mesa/r200/i915/i965: move ARB program fields into a union

[Timothy Arceri]

On Wed, 2016-11-16 at 20:41 +0000, Emil Velikov wrote:
> On 11 November 2016 at 00:45, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > 
> > It's common for games to compile 2000 programs or more so at
> > 
> > 32bits x 2000 programs x 22 fields x 2 (at least) stages
> > 
> > This should give us something like 352 kilobytes in savings.
> > ---
> At first I was going to say "sed job", only to notice that you've
> been
> in a few places fixing indentation/wrapping.
> Now that's some dedication !

Well I still used search and replace but yeah I like to skim the code
as I go to both get an idea of what its doing and to see if there are
further clean-ups possible. No point updating code if you can just drop
it etc.

> 
> > 
> > -   prog->Instructions = NULL;
> > -   prog->NumInstructions = 0;
> > -
> >     prog->SamplersUsed = shader->active_samplers;
> >     prog->ShadowSamplers = shader->shadow_samplers;
> >     prog->ExternalSamplersUsed = gl_external_samplers(shader);
> > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > index a4679e5..7a4ee36 100644
> > --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > @@ -6501,9 +6501,6 @@ get_mesa_program_tgsi(struct gl_context *ctx,
> >        _mesa_log("\n\n");
> >     }
> > 
> > -   prog->Instructions = NULL;
> > -   prog->NumInstructions = 0;
> > -
> Skimmed through all the NewProgram callback implementations and to
> check that those two are safe.
> Can you please mention that in the commit summary and/or leave it as
> separate fix ?

I'll add it to the commit message. Since we create the struct with
rzalloc this doesn't really do anything :)

> 
> Regardless of the route taken here and previous patch (but with
> LocalParams hunk properly handled here), with first 9 are
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

Great! Thanks for taking a look :)

> 
> Mildly related: why does i965 (brw_link.cpp) use
> ctx->Driver.NewProgram instead of exporting brwNewProgram and using
> it
> directly ?

Yeah that's a good question, however as you have probably now seen this
gets moved in patch 11 so I guess it doesn't matter anymore.

> 
> Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev