[Mesa-dev] [PATCH] glsl: Fix program interface queries relating to interface blocks.
Alejandro Piñeiro
apinheiro at igalia.com
Sat Dec 17 19:41:35 UTC 2016
On 17/12/16 03:35, Kenneth Graunke wrote:
> This fixes 555 dEQP tests (using the nougat-cts-dev branch), Piglit's
> arb_program_interface_query/arb_program_interface_query-resource-query,
Somewhat offtopic: FWIW, piglit tests for arb_program_interface_query
doesn't tests too much the REFERENCED_BY_XXX property (see below). I
have a wip piglit tests locally that still fails. Mentioning this
because ...
> and GL45-CTS.program_interface_query.separate-programs-{tess-control,
> tess-eval,geometry}. Only one dEQP program interface failure remains.
... as far as I see, just one CTS program interface query keeps failing
after your patch, and it is caused by some issues with
REFERENCED_BY_XXX. In any case, about this, now Im somewhat more
optimistic, after realizing (while testing your patch) that Ian Romanick
fixed GL45-CTS.geometry_shader.program_resource.program_resource, and
cleaned up referenced_by on the patch "linker: Accurately track
gl_uniform_block::stageref" (commit 084105). Will take a look to that
next Monday.
> I would have liked to split this up into several distinct changes, but
> I wasn't sure how to do that given thet tangled nature of these issues.
>
> So, the issues:
>
> * We need to treat interface blocks declared as an array of instances
> as a single block - removing the outer array. The resource list
> entry's name should not include the array length. Properties such
> as GL_ARRAY_SIZE should refer to the variable inside the block, not
> the interface block's array properties.
>
> * We need to do this prefixing even for structure variables.
>
> * We need to do this for built-ins (such as gl_PerVertex.gl_Position).
>
> * After interface array unwrapping, any variable which is an array
> should have [0] appended. It doesn't matter if it's a TCS/TES/GS
> input or TCS output - that looked like an attempt to unwrap for
> per-vertex variables, but that didn't consider per-patch variables,
> and as far as I can tell there's nothing to justify this.
>
> Several Mesa developers have suggested that Issue 16 contradicts the
> main specification,
Me included ;), although probably I misunderstood the spec as ...
> but I believe that it doesn't - the main spec just
> isn't terribly clear.
... I agree.
> The main ARB_program_interface query spec says:
>
> "* For an active interface block not declared as an array of block
> instances, a single entry will be generated, using the block name from
> the shader source.
>
> * For an active interface block declared as an array of instances,
> separate entries will be generated for each active instance. The name
> of the instance is formed by concatenating the block name, the "["
> character, an integer identifying the instance number, and the "]"
> character."
>
> Issue 16 says that built-ins should be named "gl_PerVertex.gl_Position",
> but several people suggested the second bullet above means that it
> should be named "gl_PerVertex[array length].gl_Position".
>
> There are two important things to note. Those bullet points say
> "an active interface block", while the others say "variable" or "active
> shader storage block member". They also don't mention applying the
> rules recursively (unlike the other bullets). Both suggest that
> these rules apply to blocks themselves, not members of blocks.
>
> In fact, for GL_UNIFORM_BLOCK queries, we do have "block[0]",
> "block[1]", ... resource list entries - so those rules are real,
> and actually used. So if they don't apply to block members, then how
> should members be named?
Makes sense, although in a really convoluted way, as it seems very
interpretative. But that goes back to the lack of clearness of the spec.
Perhaps (I hope?) all this work would help to clarify it.
> Unfortunately, I don't see any rules outside
> of issue 16 - where the rationale is very unclear. I hope to clarify
> the spec in the future.
Yes, I agree. Not sure what was the intention behind issue 16. Seems an
exception to the common rule, but not sure why it was needed.
>
> Cc: Ian Romanick <idr at freedesktop.org>
> Cc: Alejandro Piñeiro <apinheiro at igalia.com>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
> src/compiler/glsl/linker.cpp | 100 ++++++++++++++++++++++++-----------------
> src/mesa/main/shader_query.cpp | 92 ++++++++-----------------------------
> 2 files changed, 79 insertions(+), 113 deletions(-)
>
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 5066014..5508d58 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -3735,6 +3735,65 @@ add_shader_variable(const struct gl_context *ctx,
> bool use_implicit_location, int location,
> const glsl_type *outermost_struct_type = NULL)
> {
> + const glsl_type *interface_type = var->get_interface_type();
> +
> + if (outermost_struct_type == NULL) {
> + /* Unsized (non-patch) TCS output/TES input arrays are implicitly
> + * sized to gl_MaxPatchVertices. Internally, we shrink them to a
> + * smaller size.
> + *
> + * This can cause trouble with SSO programs. Since the TCS declares
> + * the number of output vertices, we can always shrink TCS output
> + * arrays. However, the TES might not be linked with a TCS, in
> + * which case it won't know the size of the patch. In other words,
> + * the TCS and TES may disagree on the (smaller) array sizes. This
> + * can result in the resource names differing across stages, causing
> + * SSO validation failures and other cascading issues.
> + *
> + * Expanding the array size to the full gl_MaxPatchVertices fixes
> + * these issues. It's also what program interface queries expect,
> + * as that is the official size of the array.
> + */
> + if (var->data.tess_varying_implicit_sized_array) {
> + type = resize_to_max_patch_vertices(ctx, type);
> + interface_type = resize_to_max_patch_vertices(ctx, interface_type);
> + }
> +
> + if (var->data.from_named_ifc_block) {
> + const char *interface_name = interface_type->name;
> +
> + if (interface_type->is_array()) {
> + /* Issue #16 of the ARB_program_interface_query spec says:
> + *
> + * "* If a variable is a member of an interface block without an
> + * instance name, it is enumerated using just the variable name.
> + *
> + * * If a variable is a member of an interface block with an
> + * instance name, it is enumerated as "BlockName.Member", where
> + * "BlockName" is the name of the interface block (not the
> + * instance name) and "Member" is the name of the variable."
> + *
> + * In particular, it indicates that it should be "BlockName",
> + * not "BlockName[array length]". The conformance suite and
> + * dEQP both require this behavior.
> + *
> + * Here, we unwrap the extra array level added by named interface
> + * block array lowering so we have the correct variable type. We
> + * also unwrap the interface type when constructing the name.
> + *
> + * We leave interface_type the same so that ES 3.x SSO pipeline
> + * validation can enforce the rules requiring array length to
> + * match on interface blocks.
> + */
> + type = type->fields.array;
> +
> + interface_name = interface_type->fields.array->name;
> + }
> +
> + name = ralloc_asprintf(shProg, "%s.%s", interface_name, name);
> + }
> + }
> +
> switch (type->base_type) {
> case GLSL_TYPE_STRUCT: {
> /* The ARB_program_interface_query spec says:
> @@ -3766,44 +3825,6 @@ add_shader_variable(const struct gl_context *ctx,
> }
>
> default: {
> - const glsl_type *interface_type = var->get_interface_type();
> -
> - /* Unsized (non-patch) TCS output/TES input arrays are implicitly
> - * sized to gl_MaxPatchVertices. Internally, we shrink them to a
> - * smaller size.
> - *
> - * This can cause trouble with SSO programs. Since the TCS declares
> - * the number of output vertices, we can always shrink TCS output
> - * arrays. However, the TES might not be linked with a TCS, in
> - * which case it won't know the size of the patch. In other words,
> - * the TCS and TES may disagree on the (smaller) array sizes. This
> - * can result in the resource names differing across stages, causing
> - * SSO validation failures and other cascading issues.
> - *
> - * Expanding the array size to the full gl_MaxPatchVertices fixes
> - * these issues. It's also what program interface queries expect,
> - * as that is the official size of the array.
> - */
> - if (var->data.tess_varying_implicit_sized_array) {
> - type = resize_to_max_patch_vertices(ctx, type);
> - interface_type = resize_to_max_patch_vertices(ctx, interface_type);
> - }
> -
> - /* Issue #16 of the ARB_program_interface_query spec says:
> - *
> - * "* If a variable is a member of an interface block without an
> - * instance name, it is enumerated using just the variable name.
> - *
> - * * If a variable is a member of an interface block with an instance
> - * name, it is enumerated as "BlockName.Member", where "BlockName" is
> - * the name of the interface block (not the instance name) and
> - * "Member" is the name of the variable."
> - */
> - const char *prefixed_name = (var->data.from_named_ifc_block &&
> - !is_gl_identifier(var->name))
> - ? ralloc_asprintf(shProg, "%s.%s", interface_type->name, name)
> - : name;
> -
> /* The ARB_program_interface_query spec says:
> *
> * "For an active variable declared as a single instance of a basic
> @@ -3811,8 +3832,7 @@ add_shader_variable(const struct gl_context *ctx,
> * from the shader source."
> */
> gl_shader_variable *sha_v =
> - create_shader_variable(shProg, var, prefixed_name, type,
> - interface_type,
> + create_shader_variable(shProg, var, name, type, interface_type,
> use_implicit_location, location,
> outermost_struct_type);
> if (!sha_v)
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index e4a8773..1d1616b 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -686,31 +686,14 @@ _mesa_program_resource_find_index(struct gl_shader_program *shProg,
> * ambiguous in this regard. However, either name can later be passed
> * to glGetUniformLocation (and related APIs), so there shouldn't be any
> * harm in always appending "[0]" to uniform array names.
> - *
> - * Geometry shader stage has different naming convention where the 'normal'
> - * condition is an array, therefore for variables referenced in geometry
> - * stage we do not add '[0]'.
> - *
> - * Note, that TCS outputs and TES inputs should not have index appended
> - * either.
> */
> 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;
> -
> - return add_index;
> + return res->Type != GL_TRANSFORM_FEEDBACK_VARYING;
> }
>
> /* Get name length of a program resource. This consists of
> @@ -1401,9 +1384,6 @@ validate_io(struct gl_shader_program *producer,
>
> bool valid = true;
>
> - void *name_buffer = NULL;
> - size_t name_buffer_size = 0;
> -
> gl_shader_variable const **outputs =
> (gl_shader_variable const **) calloc(producer->NumProgramResourceList,
> sizeof(gl_shader_variable *));
> @@ -1475,52 +1455,11 @@ validate_io(struct gl_shader_program *producer,
> }
> }
> } else {
> - char *consumer_name = consumer_var->name;
> -
> - if (nonarray_stage_to_array_stage &&
> - consumer_var->interface_type != NULL &&
> - consumer_var->interface_type->is_array() &&
> - !is_gl_identifier(consumer_var->name)) {
> - const size_t name_len = strlen(consumer_var->name);
> -
> - if (name_len >= name_buffer_size) {
> - free(name_buffer);
> -
> - name_buffer_size = name_len + 1;
> - name_buffer = malloc(name_buffer_size);
> - if (name_buffer == NULL) {
> - valid = false;
> - goto out;
> - }
> - }
> -
> - consumer_name = (char *) name_buffer;
> -
> - char *s = strchr(consumer_var->name, '[');
> - if (s == NULL) {
> - valid = false;
> - goto out;
> - }
> -
> - char *t = strchr(s, ']');
> - if (t == NULL) {
> - valid = false;
> - goto out;
> - }
> -
> - assert(t[1] == '.' || t[1] == '[');
> -
> - const ptrdiff_t base_name_len = s - consumer_var->name;
> -
> - memcpy(consumer_name, consumer_var->name, base_name_len);
> - strcpy(consumer_name + base_name_len, t + 1);
> - }
> -
> for (unsigned j = 0; j < num_outputs; j++) {
> const gl_shader_variable *const var = outputs[j];
>
> if (!var->explicit_location &&
> - strcmp(consumer_name, var->name) == 0) {
> + strcmp(consumer_var->name, var->name) == 0) {
> producer_var = var;
> match_index = j;
> break;
> @@ -1583,21 +1522,29 @@ validate_io(struct gl_shader_program *producer,
> * find the producer variable that goes with the consumer variable.
> */
> if (nonarray_stage_to_array_stage) {
> - if (!consumer_var->type->is_array() ||
> - consumer_var->type->fields.array != producer_var->type) {
> - valid = false;
> - goto out;
> - }
> -
> if (consumer_var->interface_type != NULL) {
> + /* the interface is the array; underlying types should match */
> + if (producer_var->type != consumer_var->type) {
> + valid = false;
> + goto out;
> + }
> +
> if (!consumer_var->interface_type->is_array() ||
> consumer_var->interface_type->fields.array != producer_var->interface_type) {
> valid = false;
> goto out;
> }
> - } else if (producer_var->interface_type != NULL) {
> - valid = false;
> - goto out;
> + } else {
> + if (producer_var->interface_type != NULL) {
> + valid = false;
> + goto out;
> + }
> +
> + if (!consumer_var->type->is_array() ||
> + consumer_var->type->fields.array != producer_var->type) {
> + valid = false;
> + goto out;
> + }
> }
> } else {
> if (producer_var->type != consumer_var->type) {
> @@ -1628,7 +1575,6 @@ validate_io(struct gl_shader_program *producer,
> }
>
> out:
> - free(name_buffer);
> free(outputs);
> return valid && num_outputs == 0;
> }
As you already mentioned, it would be good to have this patch split on
patches handling individual issues. But as I don't have any proposal
about how to do that:
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>
BR
More information about the mesa-dev
mailing list