[Mesa-dev] [PATCH 2/2] glsl: Size TCS->TES unsized arrays to gl_MaxPatchVertices for queries.
Tapani Pälli
tapani.palli at intel.com
Wed Oct 26 04:45:39 UTC 2016
On 10/26/2016 04:33 AM, Timothy Arceri wrote:
> On Tue, 2016-10-25 at 13:15 -0700, Kenneth Graunke wrote:
>> SSO validation and other program interface queries want to see that
>> unsized (non-patch) TCS output/TES input arrays are implicitly sized
>> to gl_MaxPatchVertices.
>>
>> By the time we create the program resource lists, we've sized the
>> arrays
>> to their actual size. (We try to create TCS output arrays to match
>> the
>> output patch size right away, and at this point, we should have
>> shrunk
>> TES input arrays.) One option would be to keep them sized to
>> gl_MaxPatchVertices, and defer shrinking them. But that's a big
>> change,
>> and I don't think it's a good idea.
>>
>> Instead, this patch introduces a new ir_variable flag which indicates
>> the variable is implicitly to gl_MaxPatchVertices. Then, the linker
>> munges the types when creating the resource list, ignoring the size
>> in the IR's types. Basically, lie about it for resource queries.
>> It's ugly, but I think it ought to work.
>>
>> We probably could use var->data.implicit_sized_array for this, but
>> I opted for a separate bit to try and avoid convoluting the existing
>> SSBO handling. They're similar in concept, but share none of the
>> same code...
>>
>> Fixes:
>> ES31-
>> CTS.core.tessellation_shader.single.xfb_captures_data_from_correct_st
>> age
>> and the ES32-CTS and ESEXT-CTS variants.
>>
>> Cc: mesa-stable at lists.freedesktop.org
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>> src/compiler/glsl/ast_to_hir.cpp | 3 +++
>> src/compiler/glsl/ir.h | 6 ++++++
>> src/compiler/glsl/linker.cpp | 24
>> +++++++++++++++++++---
>> src/compiler/glsl/lower_named_interface_blocks.cpp | 2 ++
>> 4 files changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index adedcbb..a9a1ba3 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -4330,6 +4330,8 @@ handle_tess_ctrl_shader_output_decl(struct
>> _mesa_glsl_parse_state *state,
>> if (var->data.patch)
>> return;
>>
>> + var->data.tess_varying_implicit_sized_array = var->type-
>>> is_unsized_array();
>> +
>> validate_layout_qualifier_vertex_count(state, loc, var,
>> num_vertices,
>> &state->tcs_output_size,
>> "tessellation control
>> shader output");
>> @@ -4366,6 +4368,7 @@ handle_tess_shader_input_decl(struct
>> _mesa_glsl_parse_state *state,
>> if (var->type->is_unsized_array()) {
>> var->type = glsl_type::get_array_instance(var->type-
>>> fields.array,
>> state->Const.MaxPatchVertices);
>> + var->data.tess_varying_implicit_sized_array = true;
>> } else if (var->type->length != state->Const.MaxPatchVertices) {
>> _mesa_glsl_error(&loc, state,
>> "per-vertex tessellation shader input arrays
>> must be "
>> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
>> index 3d28dd5..f07e3b2 100644
>> --- a/src/compiler/glsl/ir.h
>> +++ b/src/compiler/glsl/ir.h
>> @@ -833,6 +833,12 @@ public:
>> unsigned implicit_sized_array:1;
>>
>> /**
>> + * Is this a non-patch TCS output / TES input array that was
>> implicitly
>> + * sized to gl_MaxPatchVertices?
>> + */
>> + unsigned tess_varying_implicit_sized_array:1;
>> +
>> + /**
>> * Whether this is a fragment shader output implicitly
>> initialized with
>> * the previous contents of the specified render target at the
>> * framebuffer location corresponding to this shader
>> invocation.
>> diff --git a/src/compiler/glsl/linker.cpp
>> b/src/compiler/glsl/linker.cpp
>> index 116e17b5..45a6c97 100644
>> --- a/src/compiler/glsl/linker.cpp
>> +++ b/src/compiler/glsl/linker.cpp
>> @@ -3584,6 +3584,7 @@ static gl_shader_variable *
>> create_shader_variable(struct gl_shader_program *shProg,
>> const ir_variable *in,
>> const char *name, const glsl_type *type,
>> + const glsl_type *interface_type,
>> bool use_implicit_location, int location,
>> const glsl_type *outermost_struct_type)
>> {
>> @@ -3641,7 +3642,7 @@ create_shader_variable(struct gl_shader_program
>> *shProg,
>>
>> out->type = type;
>> out->outermost_struct_type = outermost_struct_type;
>> - out->interface_type = in->get_interface_type();
>> + out->interface_type = interface_type;
>> out->component = in->data.location_frac;
>> out->index = in->data.index;
>> out->patch = in->data.patch;
>> @@ -3653,6 +3654,17 @@ create_shader_variable(struct
>> gl_shader_program *shProg,
>> return out;
>> }
>>
>> +static const glsl_type *
>> +resize_to_max_patch_vertices(const struct gl_context *ctx,
>> + const glsl_type *type)
>> +{
>> + if (!type)
>> + return NULL;
>> +
>> + return glsl_type::get_array_instance(type->fields.array,
>> + ctx-
>>> Const.MaxPatchVertices);
>> +}
>> +
>> static bool
>> add_shader_variable(const struct gl_context *ctx,
>> struct gl_shader_program *shProg,
>> @@ -3699,6 +3711,12 @@ add_shader_variable(const struct gl_context
>> *ctx,
>> }
>>
>> default: {
>
> It sucks that we need to do this :(
>
> Since this is a hack and not imediatly obvious as to why we do it. It
> would be good to add a comment here. Maybe something like:
>
> /* SSO validation and other program interface queries want to see that
> * unsized (non-patch) TCS output/TES input arrays are implicitly sized
> * to gl_MaxPatchVertices.
>
> * By the time we get here, we've shrunk the arrays the to their actual
> * size. So we create new types here to lie about it for resource
> * queries.
Well ... I would not call this a *lie* but rather something like "return
the expected size", tell what the API expects the size to be
optimization is implementation detail). API expects it to match
"implementation-dependent maximum patch size" so that's what we should
return, maybe even quote the spec?
series is
Reviewed-by: Tapani Pälli <tapani.palli at intel.com>
> */
>
> Otherwise series is:
>
> Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
>
>> + const glsl_type *interface_type = var->get_interface_type();
>> + 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
>> @@ -3711,8 +3729,7 @@ add_shader_variable(const struct gl_context
>> *ctx,
>> */
>> const char *prefixed_name = (var->data.from_named_ifc_block &&
>> !is_gl_identifier(var->name))
>> - ? ralloc_asprintf(shProg, "%s.%s", var-
>>> get_interface_type()->name,
>> - name)
>> + ? ralloc_asprintf(shProg, "%s.%s", interface_type->name,
>> name)
>> : name;
>>
>> /* The ARB_program_interface_query spec says:
>> @@ -3723,6 +3740,7 @@ add_shader_variable(const struct gl_context
>> *ctx,
>> */
>> gl_shader_variable *sha_v =
>> create_shader_variable(shProg, var, prefixed_name, type,
>> + interface_type,
>> use_implicit_location, location,
>> outermost_struct_type);
>> if (!sha_v)
>> diff --git a/src/compiler/glsl/lower_named_interface_blocks.cpp
>> b/src/compiler/glsl/lower_named_interface_blocks.cpp
>> index a00e60d..53ef638 100644
>> --- a/src/compiler/glsl/lower_named_interface_blocks.cpp
>> +++ b/src/compiler/glsl/lower_named_interface_blocks.cpp
>> @@ -193,6 +193,8 @@
>> flatten_named_interface_blocks_declarations::run(exec_list
>> *instructions)
>> new_var->data.patch = iface_t-
>>> fields.structure[i].patch;
>> new_var->data.stream = var->data.stream;
>> new_var->data.how_declared = var->data.how_declared;
>> + new_var->data.tess_varying_implicit_sized_array =
>> + var->data.tess_varying_implicit_sized_array;
>> new_var->data.from_named_ifc_block = 1;
>>
>> new_var->init_interface_type(var->type);
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list