[Mesa-dev] [Mesa-stable] [PATCH 3/5] mesa: Fix add_index_to_name logic
Timothy Arceri
timothy.arceri at collabora.com
Wed Jun 1 23:25:23 UTC 2016
On Wed, 2016-06-01 at 14:55 -0700, Ian Romanick wrote:
> On 05/31/2016 04:45 PM, Ian Romanick wrote:
> >
> > On 05/31/2016 02:44 PM, Timothy Arceri wrote:
> > >
> > > On Tue, 2016-05-31 at 11:52 -0700, Ian Romanick wrote:
> > > >
> > > > From: Ian Romanick <ian.d.romanick at intel.com>
> > > >
> > > > Our piglit tests for geometry and tessellation shader inputs
> > > > were
> > > > incorrect. Array shader inputs and output should have '[0]' on
> > > > the
> > > > end
> > > > regardless of stage. In addtion, transform feedback varyings
> > > > should
> > > > not.
> > > Is there a spec quote for this? It doesn't seem right to me since
> > > for
> > > arrays of arrays would that mean we should end up with gs inputs
> > > like
> > > this
> > Here are all the rules that I think applies:
> >
> > * For an active variable declared as an array of basic types,
> > a single
> > entry will be generated, with its name string formed by
> > concatenating
> > the name of the array and the string "[0]".
> >
> > * For an active variable declared as a structure, a separate
> > entry will
> > be generated for each active structure member. The name of
> > each entry
> > is formed by concatenating the name of the structure, the
> > "."
> > character, and the name of the structure member. If a
> > structure
> > member to enumerate is itself a structure or array, these
> > enumeration
> > rules are applied recursively.
> >
> > * For an active variable declared as an array of an aggregate
> > data type
> > (structures or arrays), a separate entry will be generated
> > for each
> > active array element, unless noted immediately below. The
> > name of
> > each entry is formed by concatenating the name of the
> > array, the "["
> > character, an integer identifying the element number, and
> > the "]"
> > character. These enumeration rules are applied
> > recursively, treating
> > each enumerated array element as a separate active
> > variable.
> >
> > >
> > > input_name[0][0]
> > > input_name[...][0]
> > > input_name[num_vertices-1][0]
> > Yes, this is correct. We don't do this with or without this patch.
> > I
> > don't know of any tests that exercise this. Alas.
> >
> > >
> > > otherwise
> > >
> > > in vec4 input1[];
> > > and
> > > in vec4 input2[][3];
> > >
> > > Would both end up as:
> > > input1[0]
> > > input2[0]
> > >
> > > >
> > > >
> > > > Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> > > > Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> > > > ---
> > > > src/mesa/main/shader_query.cpp | 23 ++++++++++-------------
> > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/src/mesa/main/shader_query.cpp
> > > > b/src/mesa/main/shader_query.cpp
> > > > index eec933c..f4b7243 100644
> > > > --- a/src/mesa/main/shader_query.cpp
> > > > +++ b/src/mesa/main/shader_query.cpp
> > > > @@ -696,20 +696,17 @@ _mesa_program_resource_find_index(struct
> > > > gl_shader_program *shProg,
> > > > static bool
> > > > add_index_to_name(struct gl_program_resource *res)
> > > > {
> > > > - bool add_index = !((res->Type == GL_PROGRAM_INPUT &&
> > > > - res->StageReferences & (1 <<
> > > > MESA_SHADER_GEOMETRY |
> > > > - 1 <<
> > > > MESA_SHADER_TESS_CTRL |
> > > > - 1 <<
> > > > MESA_SHADER_TESS_EVAL)) ||
> > > > - (res->Type == GL_PROGRAM_OUTPUT &&
> > > > - res->StageReferences & 1 <<
> > > > MESA_SHADER_TESS_CTRL));
> > > > -
> > > > - /* Transform feedback varyings have array index already
> > > > appended
> > > > - * in their names.
> > > > - */
> > > > - if (res->Type == GL_TRANSFORM_FEEDBACK_VARYING)
> > > > - add_index = false;
> > > > + if (res->Type != GL_PROGRAM_INPUT && res->Type !=
> > > > GL_PROGRAM_OUTPUT)
> > > > + return res->Type != GL_TRANSFORM_FEEDBACK_VARYING;
> > > I'm slighlty confused by this. When does this return true? And
> > > for
> > > transform feedback wont this always end up us false?
> > >
> > > So isn't it just
> > >
> > > if (res->Type == GL_TRANSFORM_FEEDBACK_VARYING)
> > > return false;
> > I thought I tried that but it regressed some dEQP tests. I'll
> > double
> > check.
> It makes a huge pile of dEQP tests fail because lots of things,
> including UBOs and SSBOs, come through this function. Inputs and
> outputs get some special treatment because of the array-of-interface
> handing, and xfb variables never get the [0] suffix. Everything else
> gets the [0] suffix.
Hmm ... I guess thats because lowering of block works different to
interface blocks.
I've taken another look at the code and it seems to me that the change
should be made in _mesa_program_resource_array_size() rather than here:
e.g
case GL_PROGRAM_INPUT:
case GL_PROGRAM_OUTPUT: {
const gl_shader_variable *const var = RESOURCE_VAR(res);
if (var->interface_type != NULL && var->interface_type-
>is_array())
/* TODO: Add support for AoA interface blocks */
return var->type->fields.array->length;
else
return var->type->length;
}
Then this whole function could just be:
return res->Type != GL_TRANSFORM_FEEDBACK_VARYING;
>
> >
> > >
> > > >
> > > > +
> > > > + const gl_shader_variable *const var = RESOURCE_VAR(res);
> > > >
> > > > - return add_index;
> > > > + assert(var->type->is_array());
> > > > +
> > > > + if (var->interface_type != NULL && var->interface_type-
> > > > >
> > > > > is_array())
> > > > + return var->type->fields.array->is_array();
> > > > +
> > > So I'm assuming your doing this since after lowering block[3].foo
> > > we
> > > end up with foo[3]. However something like block[3].foo[2] will
> > > end up
> > > as foo[3][2] and blocks can also be arrays of arrays so I'm not
> > > sure
> > > this will work.
> > Correct. I had forgotten about blocks being arrays-of-
> > arrays. That
> > should be easy enough to fix.
> >
> > >
> > > >
> > > > + return true;
> > > > }
> > > >
> > > > /* Get name length of a program resource. This consists of
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
More information about the mesa-dev
mailing list