[Mesa-dev] [PATCH 3/3] mesa: Clean up gl_array_object::_MaxElement computation.

[Eric Anholt]

On Thu, 29 Dec 2011 23:14:25 +0100, Mathias Fröhlich <Mathias.Froehlich at gmx.net> wrote:
> Use a bitmask approach to compute gl_array_object::_MaxElement.
> To make this work reliably depending on the shader type actually used,
> make use of the newly introduced typed bitmaks getters.
> With this change I gain about 5% draw time on some osgviewer examples.

Unless I'm reading wrong, there's some actual behavior difference here.
Suppose you're using an ARB VP, so VertexProgram._Enabled is true, and
your program uses texcoord 0 and generic 8.  Before, the logic was:

> -   /* 8..15 */
> -   for (i = 0; i < VERT_ATTRIB_TEX_MAX; i++) {
> -      if (ctx->VertexProgram._Enabled
> -          && arrayObj->VertexAttrib[VERT_ATTRIB_GENERIC8 + i].Enabled) {
> -         min = update_min(min, &arrayObj->VertexAttrib[VERT_ATTRIB_GENERIC8 + 
> i]);
> -      }
> -      else if (i < ctx->Const.MaxTextureCoordUnits
> -               && arrayObj->VertexAttrib[VERT_ATTRIB_TEX(i)].Enabled) {
> -         min = update_min(min, &arrayObj->VertexAttrib[VERT_ATTRIB_TEX(i)]);
> -      }
> -   }

> -   /* 16..31 */
> -   if (ctx->VertexProgram._Current) {
> -      for (i = 0; i < VERT_ATTRIB_GENERIC_MAX; i++) {
> -         if (arrayObj->VertexAttrib[VERT_ATTRIB_GENERIC(i)].Enabled) {
> -            min = update_min(min, &arrayObj-
> >VertexAttrib[VERT_ATTRIB_GENERIC(i)]);
> -         }
> -      }
> -   }

So we wouldn't look at the size of the texcoord, because the generic
took precedence.  Now, we just use the bits in:

+static inline GLbitfield64
+_mesa_array_object_get_enabled_arb(const struct gl_array_object *arrayObj)
+{
+   GLbitfield64 enabled = arrayObj->_Enabled;
+   return enabled & ~(VERT_BIT_POS & (enabled >> VERT_ATTRIB_GENERIC0));
+}
+

So we end up looking at the texcoord's size.  I think this is intended,
and is a fix, but it was surprising to see a change in a cleanup patch.

Also, I think that helper would be easier to read as;

+   GLbitfield64 enabled = arrayObj->_Enabled;
+
+   if (enabled & VERT_ATTRIB_GENERIC0)
+      enabled &= ~VERT_BIT_POS;
+
+   return enabled;

One last little bit of review:

>  static void
>  update_arrays( struct gl_context *ctx )
>  {
>     struct gl_array_object *arrayObj = ctx->Array.ArrayObj;
> -   GLuint i, min = ~0;
[many lines of minus]
> -   /* _MaxElement is one past the last legal array element */
> -   arrayObj->_MaxElement = min;
> +   _mesa_update_array_object_max_element(ctx, arrayObj);
>  }

I think this function could be removed and
_mesa_update_array_object_max_element called directly now.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111230/67cd7e72/attachment.pgp>