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.roman...@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.roman...@intel.com> > > > > Cc: "12.0" <mesa-sta...@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > _______________________________________________ > mesa-stable mailing list > mesa-sta...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-stable _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev