[Mesa-dev] [PATCH 1/3] glsl: XFB TSC per-vertex output varyings match as not declared as arrays

[Chema Casanova]

On 21/11/18 20:04, Ilia Mirkin wrote:
> On Wed, Nov 21, 2018 at 1:45 PM Jose Maria Casanova Crespo
> <jmcasanova at igalia.com> wrote:
>>
>> Recent change on OpenGL CTS ("Use non-arrayed varying name for TCS blocks")
>> on KHR-GL*.tessellation_shader.single.xfb_captures_data_from_correct_stage
>> tests changed how to name per-vertex Tessellation Control Shader output
>> varyings in transform feedback using interface block as "BLOCK_INOUT.value"
>> rather than "BLOCK_INOUT[0].value"
>>
>> So Tessellation control shader per-vertex output variables and blocks that
>> are required to be declared as arrays, with each element representing output
>> values for a single vertex of a multi-vertex primitive are expected to be
>> named as they were not declared as arrays.
>>
>> This patch adds a new is_xfb_per_vertex_output flag at ir_variable level so
>> we mark when an ir_variable is an per-vertex TCS output varying. So we
>> treat it in terms on XFB its naming as a non array variable.
>>
>> As we don't support NV_gpu_shader5, so PATCHES mode is not accepted as
>> primitiveMode parameter of BeginTransformFeedback the test expects a
>> failure as we can use the XFB results.
>>
>> This patch uncovers that we were passing the GLES version of the tests
>> because candidates naming didn't match, not because on GLES the Tessellation
>> Control stage varyings shouldn't be XFB candidates in any case. This
>> is addressed in the following patch.
>>
>> Fixes: KHR-GL4*.tessellation_shader.single.xfb_captures_data_from_correct_stage
>>
>> Cc: mesa-stable at lists.freedesktop.org
>> ---
>>  src/compiler/glsl/ir.cpp            | 1 +
>>  src/compiler/glsl/ir.h              | 6 ++++++
>>  src/compiler/glsl/link_uniforms.cpp | 6 ++++--
>>  src/compiler/glsl/link_varyings.cpp | 8 +++++++-
>>  4 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ir.cpp b/src/compiler/glsl/ir.cpp
>> index 1d1a56ae9a5..582111d71f5 100644
>> --- a/src/compiler/glsl/ir.cpp
>> +++ b/src/compiler/glsl/ir.cpp
>> @@ -1750,6 +1750,7 @@ ir_variable::ir_variable(const struct glsl_type *type, const char *name,
>>     this->data.fb_fetch_output = false;
>>     this->data.bindless = false;
>>     this->data.bound = false;
>> +   this->data.is_xfb_per_vertex_output = false;
>>
>>     if (type != NULL) {
>>        if (type->is_interface())
>> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
>> index f478b29a6b5..e09f053b77c 100644
>> --- a/src/compiler/glsl/ir.h
>> +++ b/src/compiler/glsl/ir.h
>> @@ -766,6 +766,12 @@ public:
>>         */
>>        unsigned is_xfb_only:1;
>>
>> +      /**
>> +       * Is this varying a TSC per-vertex output candidate for transform
> 
> TCS?


Yes. I've fixed it locally at the commit summary too.


>> +       * feedback?
>> +       */
>> +      unsigned is_xfb_per_vertex_output:1;
>> +
>>        /**
>>         * Was a transfor feedback buffer set in the shader?
> 
> ugh, not your problem, but "transform" :(
> 
>>         */
>> diff --git a/src/compiler/glsl/link_uniforms.cpp b/src/compiler/glsl/link_uniforms.cpp
>> index 63e688b19a7..547da68e216 100644
>> --- a/src/compiler/glsl/link_uniforms.cpp
>> +++ b/src/compiler/glsl/link_uniforms.cpp
>> @@ -72,8 +72,10 @@ program_resource_visitor::process(ir_variable *var, bool use_std430_as_default)
>>           get_internal_ifc_packing(use_std430_as_default) :
>>        var->type->get_internal_ifc_packing(use_std430_as_default);
>>
>> -   const glsl_type *t =
>> -      var->data.from_named_ifc_block ? var->get_interface_type() : var->type;
>> +   const glsl_type *t = var->data.from_named_ifc_block ?
>> +      (var->data.is_xfb_per_vertex_output ?
>> +       var->get_interface_type()->without_array() :
>> +       var->get_interface_type()) : var->type;
>>     const glsl_type *t_without_array = t->without_array();
>>
>>     /* false is always passed for the row_major parameter to the other
>> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
>> index 52e493cb599..1964dcc0a22 100644
>> --- a/src/compiler/glsl/link_varyings.cpp
>> +++ b/src/compiler/glsl/link_varyings.cpp
>> @@ -2150,7 +2150,10 @@ private:
>>        tfeedback_candidate *candidate
>>           = rzalloc(this->mem_ctx, tfeedback_candidate);
>>        candidate->toplevel_var = this->toplevel_var;
>> -      candidate->type = type;
>> +      if (this->toplevel_var->data.is_xfb_per_vertex_output)
>> +         candidate->type = type->without_array();
>> +      else
>> +         candidate->type = type;
>>        candidate->offset = this->varying_floats;
>>        _mesa_hash_table_insert(this->tfeedback_candidates,
>>                                ralloc_strdup(this->mem_ctx, name),
>> @@ -2499,6 +2502,9 @@ assign_varying_locations(struct gl_context *ctx,
>>
>>           if (num_tfeedback_decls > 0) {
>>              tfeedback_candidate_generator g(mem_ctx, tfeedback_candidates);
>> +            if (producer->Stage == MESA_SHADER_TESS_CTRL &&
>> +                !output_var->data.patch)
>> +               output_var->data.is_xfb_per_vertex_output = true;
>>              g.process(output_var);
>>           }
>>
>> --
>> 2.19.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>