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. >>> + >>> + 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-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev