[Mesa-dev] [PATCH] glsl: Update ES 3.2 shader output restrictions.

[Iago Toral]

On Mon, 2017-01-02 at 03:32 -0800, Kenneth Graunke wrote:
> This disallows fancy varyings in tessellation and geometry shaders,
> as required by ES 3.2.
> 
> Fixes:
> dEQP-
> GLES31.functional.tessellation.user_defined_io.negative.per_patch_arr
> ay_of_structs
> dEQP-
> GLES31.functional.tessellation.user_defined_io.negative.per_patch_str
> ucts_containing_arrays
> 
> (Not a candidate for stable branches as it only disallows things
> which
> should be working as desktop GL allows them.)
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 38
> +++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index 9c633863b19..762f0a88bd3 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -5039,6 +5039,24 @@ ast_declarator_list::hir(exec_list
> *instructions,
>            *     * A matrix
>            *     * A structure
>            *     * An array of array
> +          *
> +          * ES 3.20 updates this to apply to tessellation and
> geometry shaders
> +          * as well.  Because there are per-vertex arrays in the new
> stages,
> +          * it strikes the "array of..." rules and replaces them
> with these:
> +          *
> +          *     * For per-vertex-arrayed variables (applies to
> tessellation
> +          *       control, tessellation evaluation and geometry
> shaders):
> +          *
> +          *       * Per-vertex-arrayed arrays of arrays
> +          *       * Per-vertex-arrayed arrays of structures
> +          *
> +          *     * For non-per-vertex-arrayed variables:
> +          *
> +          *       * An array of arrays
> +          *       * An array of structures
> +          *
> +          * which basically says to unwrap the per-vertex aspect and
> apply
> +          * the old rules.
>            */
>           if (state->es_shader) {
>              if (var->type->is_array() &&
> @@ -5048,17 +5066,23 @@ ast_declarator_list::hir(exec_list
> *instructions,
>                                  "cannot have an array of arrays",
>                                  _mesa_shader_stage_to_string(state-
> >stage));
>              }
> -            if (state->stage == MESA_SHADER_VERTEX) {
> -               if (var->type->is_array() &&
> -                   var->type->fields.array->is_record()) {
> +            if (state->stage <= MESA_SHADER_GEOMETRY) {
> +               const glsl_type *type = var->type;
> +
> +               if (state->stage == MESA_SHADER_TESS_CTRL &&
> +                   !var->data.patch && var->type->is_array()) {
> +                  type = var->type->fields.array;
> +               }
> +
> +               if (type->is_array() && type->fields.array-
> >is_record()) {
>                    _mesa_glsl_error(&loc, state,
>                                     "vertex shader output "
>                                     "cannot have an array of
> structs");

Since this code now runs for stages other than vertex I think we want
to fix the error message to use _mesa_shader_stage_to_string()  instead
of the hard coded 'vertex' string.

>                 }
> -               if (var->type->is_record()) {
> -                  for (unsigned i = 0; i < var->type->length; i++) {
> -                     if (var->type->fields.structure[i].type-
> >is_array() ||
> -                         var->type->fields.structure[i].type-
> >is_record())
> +               if (type->is_record()) {
> +                  for (unsigned i = 0; i < type->length; i++) {
> +                     if (type->fields.structure[i].type->is_array()
> ||
> +                         type->fields.structure[i].type-
> >is_record())
>                          _mesa_glsl_error(&loc, state,
>                                           "vertex shader output
> cannot have a "
>                                           "struct that contains an "

Same thing here.

Otherwise, it looks good to me:

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>