On 12/16/2016 09:35 PM, 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,
> and GL45-CTS.program_interface_query.separate-programs-{tess-control,
> tess-eval,geometry}. Only one dEQP program interface failure remains.
>
> 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, but I believe that it doesn't - the main spec just
> isn't terribly clear. 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? 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.
Based on my understanding, something like
out Vertex {
vec4 p;
vec4 c[3];
} vertex[2];
in a vertex shader should produce
Vertex[0].p
Vertex[0].c[0] (with GL_ARRAY_SIZE = 3)
Vertex[1].p
Vertex[1].c[0] (with GL_ARRAY_SIZE = 3)
This is definitely what we would produce for a uniform block.
What I've never understood is why that isn't done for gl_PerVertex stage
boundaries where gl_PerVertex is explicitly arrayed.
I think this patch is good for now. I like that it brings all the
strange edge-case handling into one place.
Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
> Cc: Ian Romanick <i...@freedesktop.org>
> Cc: Alejandro Piñeiro <apinhe...@igalia.com>
> Signed-off-by: Kenneth Graunke <kenn...@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;
> }
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
