[Mesa-stable] [PATCH] glsl: Handle attribute aliasing in attribute storage limit check.

[Kenneth Graunke]

On Wednesday, September 02, 2015 02:35:22 PM Ilia Mirkin wrote:
> On Wed, Sep 2, 2015 at 2:20 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > In various versions of OpenGL and GLSL, it's possible to declare
> > multiple VS input variables with aliasing attribute locations.
> >
> > So, when computing the storage requirements for vertex attributes,
> > we can't simply add up the sizes.  Instead, we need to look at the
> > enabled slots.
> >
> > This patch begins tracking which attributes are double types that
> > are larger than 128-bits (i.e. take up two vec4 slots).  We then
> > count normal attributes once, and count the double-size attributes
> > a second time.
> >
> > Fixes deQP functional.attribute_location.bind_aliasing.max_cond_* tests
> > on i965, which regressed with commit ad208d975a6d3aebe14f7c2c16039ee20.
> 
> This is all subtle stuff. I'd feel a whole lot better if there were
> some piglit coverage for this.
> 
> >
> > Cc: "10.6 11.0" <mesa-stable at lists.freedesktop.org>
> > Cc: Matt Turner <mattst88 at gmail.com.
> > Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> > Cc: Dave Airlie <airlied at gmail.com>
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/glsl/linker.cpp | 64 ++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 36 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > index 47f7d25..934062f 100644
> > --- a/src/glsl/linker.cpp
> > +++ b/src/glsl/linker.cpp
> > @@ -2339,6 +2339,7 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
> >      */
> >     unsigned used_locations = (max_index >= 32)
> >        ? ~0 : ~((1 << max_index) - 1);
> > +   unsigned double_storage_locations = 0;
> >
> >     assert((target_index == MESA_SHADER_VERTEX)
> >           || (target_index == MESA_SHADER_FRAGMENT));
> > @@ -2452,34 +2453,6 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
> >
> >        const unsigned slots = var->type->count_attribute_slots();
> >
> > -      /* From GL4.5 core spec, section 11.1.1 (Vertex Attributes):
> > -       *
> > -       * "A program with more than the value of MAX_VERTEX_ATTRIBS active
> > -       * attribute variables may fail to link, unless device-dependent
> > -       * optimizations are able to make the program fit within available
> > -       * hardware resources. For the purposes of this test, attribute variables
> > -       * of the type dvec3, dvec4, dmat2x3, dmat2x4, dmat3, dmat3x4, dmat4x3,
> > -       * and dmat4 may count as consuming twice as many attributes as equivalent
> > -       * single-precision types. While these types use the same number of
> > -       * generic attributes as their single-precision equivalents,
> > -       * implementations are permitted to consume two single-precision vectors
> > -       * of internal storage for each three- or four-component double-precision
> > -       * vector."
> > -       * Until someone has a good reason in Mesa, enforce that now.
> > -       */
> > -      if (target_index == MESA_SHADER_VERTEX) {
> > -        total_attribs_size += slots;
> > -        if (var->type->without_array() == glsl_type::dvec3_type ||
> > -            var->type->without_array() == glsl_type::dvec4_type ||
> > -            var->type->without_array() == glsl_type::dmat2x3_type ||
> > -            var->type->without_array() == glsl_type::dmat2x4_type ||
> > -            var->type->without_array() == glsl_type::dmat3_type ||
> > -            var->type->without_array() == glsl_type::dmat3x4_type ||
> > -            var->type->without_array() == glsl_type::dmat4x3_type ||
> > -            var->type->without_array() == glsl_type::dmat4_type)
> > -           total_attribs_size += slots;
> > -      }
> > -
> >        /* If the variable is not a built-in and has a location statically
> >         * assigned in the shader (presumably via a layout qualifier), make sure
> >         * that it doesn't collide with other assigned locations.  Otherwise,
> > @@ -2594,6 +2567,38 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
> >             }
> >
> >             used_locations |= (use_mask << attr);
> > +
> > +            /* From the GL 4.5 core spec, section 11.1.1 (Vertex Attributes):
> > +             *
> > +             * "A program with more than the value of MAX_VERTEX_ATTRIBS
> > +             *  active attribute variables may fail to link, unless
> > +             *  device-dependent optimizations are able to make the program
> > +             *  fit within available hardware resources. For the purposes
> > +             *  of this test, attribute variables of the type dvec3, dvec4,
> > +             *  dmat2x3, dmat2x4, dmat3, dmat3x4, dmat4x3, and dmat4 may
> > +             *  count as consuming twice as many attributes as equivalent
> > +             *  single-precision types. While these types use the same number
> > +             *  of generic attributes as their single-precision equivalents,
> > +             *  implementations are permitted to consume two single-precision
> > +             *  vectors of internal storage for each three- or four-component
> > +             *  double-precision vector."
> > +             *
> > +             * Mark this attribute slot as taking up twice as much space
> > +             * so we can count it properly against limits.  According to
> > +             * issue (3) of the GL_ARB_vertex_attrib_64bit behavior, this
> > +             * is optional behavior, but it seems preferable.
> > +             */
> > +            const glsl_type *type = var->type->without_array();
> > +            if (type == glsl_type::dvec3_type ||
> > +                type == glsl_type::dvec4_type ||
> > +                type == glsl_type::dmat2x3_type ||
> > +                type == glsl_type::dmat2x4_type ||
> > +                type == glsl_type::dmat3_type ||
> > +                type == glsl_type::dmat3x4_type ||
> > +                type == glsl_type::dmat4x3_type ||
> > +                type == glsl_type::dmat4_type) {
> > +               double_storage_locations |= (use_mask << attr);
> > +            }
> >          }
> >
> >          continue;
> > @@ -2605,6 +2610,9 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
> >     }
> >
> >     if (target_index == MESA_SHADER_VERTEX) {
> > +      unsigned total_attribs_size =
> > +         _mesa_bitcount(used_locations & ((1 << max_index) - 1)) +
> > +         _mesa_bitcount(double_storage_locations);
> 
> Does this also need the & ((1 << max_index) - 1) treatment? [Well, not
> _need_ but seems like it'd avoid needlessly counting things?]

Should be fine.  The masking off here is actually to undo the earlier
code:

   /* Mark invalid locations as being used.
    */
   unsigned used_locations = (max_index >= 32)
      ? ~0 : ~((1 << max_index) - 1);

i.e. we only want to count real attributes.  I initialized my new
bitfield to 0, so we only ever counted real attributes in the first
place.

I thought about initializing them both to 0 and adding in invalid
locations after this check, but this was less invasive, and so
required less confidence in what I was doing :)

> >        if (total_attribs_size > max_index) {
> >          linker_error(prog,
> >                       "attempt to use %d vertex attribute slots only %d available ",
> > --
> > 2.5.0
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20150902/d5ea4fb3/attachment.sig>