[Mesa-dev] [PATCH 12/30] mesa: Change redundant code into loops in texstate.c.

[Paul Berry]

On 10 January 2014 11:41, Kenneth Graunke <kenneth at whitecape.org> wrote:

> On 01/09/2014 06:19 PM, Paul Berry wrote:
> > This is possible now that ctx->Shader.CurrentProgram is an array.
> > ---
> >  src/mesa/main/texstate.c | 75
> +++++++++++++++++++-----------------------------
> >  1 file changed, 29 insertions(+), 46 deletions(-)
> >
> > diff --git a/src/mesa/main/texstate.c b/src/mesa/main/texstate.c
> > index b9c76da..905a9d5 100644
> > --- a/src/mesa/main/texstate.c
> > +++ b/src/mesa/main/texstate.c
> > @@ -526,27 +526,20 @@ static void
> >  update_texture_state( struct gl_context *ctx )
> >  {
> >     GLuint unit;
> > -   struct gl_program *fprog = NULL;
> > -   struct gl_program *vprog = NULL;
> > -   struct gl_program *gprog = NULL;
> > +   struct gl_program *prog[MESA_SHADER_STAGES];
> >     GLbitfield enabledFragUnits = 0x0;
> > -
> > -   if (ctx->Shader.CurrentProgram[MESA_SHADER_VERTEX] &&
> > -       ctx->Shader.CurrentProgram[MESA_SHADER_VERTEX]->LinkStatus) {
> > -      vprog =
> ctx->Shader.CurrentProgram[MESA_SHADER_VERTEX]->_LinkedShaders[MESA_SHADER_VERTEX]->Program;
> > -   }
> > -
> > -   if (ctx->Shader.CurrentProgram[MESA_SHADER_GEOMETRY] &&
> > -       ctx->Shader.CurrentProgram[MESA_SHADER_GEOMETRY]->LinkStatus) {
> > -      gprog =
> ctx->Shader.CurrentProgram[MESA_SHADER_GEOMETRY]->_LinkedShaders[MESA_SHADER_GEOMETRY]->Program;
> > -   }
> > -
> > -   if (ctx->Shader.CurrentProgram[MESA_SHADER_FRAGMENT] &&
> > -       ctx->Shader.CurrentProgram[MESA_SHADER_FRAGMENT]->LinkStatus) {
> > -      fprog =
> ctx->Shader.CurrentProgram[MESA_SHADER_FRAGMENT]->_LinkedShaders[MESA_SHADER_FRAGMENT]->Program;
> > -   }
> > -   else if (ctx->FragmentProgram._Enabled) {
> > -      fprog = &ctx->FragmentProgram.Current->Base;
> > +   int i;
> > +
> > +   for (i = 0; i < MESA_SHADER_STAGES; i++) {
> > +      if (ctx->Shader.CurrentProgram[i] &&
> > +          ctx->Shader.CurrentProgram[i]->LinkStatus) {
> > +         prog[i] =
> ctx->Shader.CurrentProgram[i]->_LinkedShaders[i]->Program;
> > +      } else {
> > +         if (i == MESA_SHADER_FRAGMENT && ctx->FragmentProgram._Enabled)
> > +            prog[i] = &ctx->FragmentProgram.Current->Base;
> > +         else
> > +            prog[i] = NULL;
> > +      }
> >     }
> >
> >     /* TODO: only set this if there are actual changes */
> > @@ -562,9 +555,7 @@ update_texture_state( struct gl_context *ctx )
> >      */
> >     for (unit = 0; unit < ctx->Const.MaxCombinedTextureImageUnits;
> unit++) {
> >        struct gl_texture_unit *texUnit = &ctx->Texture.Unit[unit];
> > -      GLbitfield enabledVertTargets = 0x0;
> > -      GLbitfield enabledFragTargets = 0x0;
> > -      GLbitfield enabledGeomTargets = 0x0;
> > +      GLbitfield enabledTargetsByStage[MESA_SHADER_STAGES];
> >        GLbitfield enabledTargets = 0x0;
> >        GLuint texIndex;
> >
> > @@ -574,25 +565,16 @@ update_texture_state( struct gl_context *ctx )
> >         * by a fragment program/program.  When multiple flags are set,
> we'll
> >         * settle on the one with highest priority (see below).
> >         */
> > -      if (vprog) {
> > -         enabledVertTargets |= vprog->TexturesUsed[unit];
> > +      for (i = 0; i < MESA_SHADER_STAGES; i++) {
> > +         if (prog[i])
> > +            enabledTargetsByStage[i] = prog[i]->TexturesUsed[unit];
> > +         else if (i == MESA_SHADER_FRAGMENT)
> > +            enabledTargetsByStage[i] = texUnit->Enabled;
> > +         else
> > +            enabledTargetsByStage[i] = 0;
> > +         enabledTargets |= enabledTargetsByStage[i];
>
> This doesn't look equivalent.  Previously,
> enabled{Vert,Frag,Geom}Targets would accumulate bits over every
> iteration through units.  Now, enabledTargetsByStage only reflects the
> most recent unit.
>
> enabledTargets still accumulates properly, but you use
> enabledTargetsByStage below, so I think it needs to as well.
>
> You just need to use |= in both places, like the old code.
>

It's definitely possible that I got something wrong in this patch--I didn't
completely grok what the code was doing and instead strived to just
preserve the existing semantics.

But I'm pretty sure I got the enabledTargetsByStage part right.  Here's
what the code used to look like--the enabled{Vert,Frag,Geom}Targets
variables were all reset to 0 every time through the loop:

   for (unit = 0; unit < ctx->Const.MaxCombinedTextureImageUnits; unit++) {
      struct gl_texture_unit *texUnit = &ctx->Texture.Unit[unit];
      GLbitfield enabledVertTargets = 0x0;
      GLbitfield enabledFragTargets = 0x0;
      GLbitfield enabledGeomTargets = 0x0;
      GLbitfield enabledTargets = 0x0;
      GLuint texIndex;

      /* Get the bitmask of texture target enables.
       * enableBits will be a mask of the TEXTURE_*_BIT flags indicating
       * which texture targets are enabled (fixed function) or referenced
       * by a fragment program/program.  When multiple flags are set, we'll
       * settle on the one with highest priority (see below).
       */
      if (vprog) {
         enabledVertTargets |= vprog->TexturesUsed[unit];
      }

      if (gprog) {
         enabledGeomTargets |= gprog->TexturesUsed[unit];
      }

      if (fprog) {
         enabledFragTargets |= fprog->TexturesUsed[unit];
      }
      else {
         /* fixed-function fragment program */
         enabledFragTargets |= texUnit->Enabled;
      }

      enabledTargets = enabledVertTargets | enabledFragTargets |
                       enabledGeomTargets;

      ...
   }

I don't know if that was intentional or a bug, but I'm pretty sure my patch
preserves the behaviour.


>
> Also, I just want to say - this code is absurdly confusing.  Always has
> been.  I don't envy you having to touch it. :)
>

You can say that again :)  Hopefully my patch doesn't make the situation
any worse.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140110/eb85ccfe/attachment-0001.html>