On Wed, Sep 2, 2015 at 2:40 PM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Wednesday, September 02, 2015 02:35:22 PM Ilia Mirkin wrote:
>> On Wed, Sep 2, 2015 at 2:20 PM, Kenneth Graunke <kenn...@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-sta...@lists.freedesktop.org>
>> > Cc: Matt Turner <matts...@gmail.com.
>> > Cc: Ilia Mirkin <imir...@alum.mit.edu>
>> > Cc: Dave Airlie <airl...@gmail.com>
>> > Signed-off-by: Kenneth Graunke <kenn...@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 :)
Ah right, didn't look at enough of the original code. This looks
generally reasonable... might be nice to run the fp64 tests on
softpipe to make sure you didn't break anything
(LIBGL_ALWAYS_SOFTWARE=1 GALLIUM_DRIVER=softpipe should force it).
Assuming nothing fails in there,
Reviewed-by: Ilia Mirkin <imir...@alum.mit.edu>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
