[Mesa-dev] [PATCH 1/2] glsl: allow inputs in interface blocks in interpolateAt*

[Ian Romanick]

On 06/16/2017 01:37 PM, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> Since interface blocks are simply considered groupings of input variables,
> this is allowed.
> 
> var->data.must_be_shader_input is now determined on-the-fly after lowering
> interface blocks, since we don't want to disable varying packing for an
> entire block just because one input in it is used in interpolateAt*.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101378

I looked at the bug, but I haven't looked at the piglit list... is there
a test for this?  We should add one.

I /think/ this should also be tagged for stable.

> ---
>  src/compiler/glsl/ast_function.cpp                 |  7 +++++--
>  src/compiler/glsl/lower_named_interface_blocks.cpp | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
> index 2d156ae..11897f7 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -221,29 +221,32 @@ verify_parameter_modes(_mesa_glsl_parse_state *state,
>                                  formal->name);
>                 return false;
>              }
>              val = ((ir_swizzle *)val)->val;
>           }
>  
>           while (val->ir_type == ir_type_dereference_array) {
>              val = ((ir_dereference_array *)val)->array;
>           }
>  
> +         if (const ir_dereference_record *deref = val->as_dereference_record()) {
> +            if (deref->record->type->is_interface())
> +               val = deref->record;
> +         }
> +
>           if (!val->as_dereference_variable() ||
>               val->variable_referenced()->data.mode != ir_var_shader_in) {
>              _mesa_glsl_error(&loc, state,
>                               "parameter `%s` must be a shader input",
>                               formal->name);
>              return false;
>           }
> -
> -         val->variable_referenced()->data.must_be_shader_input = 1;

I think I'd like to leave this here for non-interface blocks.  I could
envision someone changing the linker to not call
lower_named_interface_blocks for shaders that can't have interface
blocks... then things would break in hard to find ways.

>        }
>  
>        /* Verify that 'out' and 'inout' actual parameters are lvalues. */
>        if (formal->data.mode == ir_var_function_out
>            || formal->data.mode == ir_var_function_inout) {
>           const char *mode = NULL;
>           switch (formal->data.mode) {
>           case ir_var_function_out:   mode = "out";   break;
>           case ir_var_function_inout: mode = "inout"; break;
>           default:                    assert(false);  break;
> diff --git a/src/compiler/glsl/lower_named_interface_blocks.cpp b/src/compiler/glsl/lower_named_interface_blocks.cpp
> index a00e60d..52e0571 100644
> --- a/src/compiler/glsl/lower_named_interface_blocks.cpp
> +++ b/src/compiler/glsl/lower_named_interface_blocks.cpp
> @@ -108,20 +108,21 @@ public:
>  
>     flatten_named_interface_blocks_declarations(void *mem_ctx)
>        : mem_ctx(mem_ctx),
>          interface_namespace(NULL)
>     {
>     }
>  
>     void run(exec_list *instructions);
>  
>     virtual ir_visitor_status visit_leave(ir_assignment *);
> +   virtual ir_visitor_status visit_leave(ir_expression *);
>     virtual void handle_rvalue(ir_rvalue **rvalue);
>  };
>  
>  } /* anonymous namespace */
>  
>  void
>  flatten_named_interface_blocks_declarations::run(exec_list *instructions)
>  {
>     interface_namespace = _mesa_hash_table_create(NULL, _mesa_key_hash_string,
>                                                   _mesa_key_string_equal);
> @@ -231,20 +232,37 @@ flatten_named_interface_blocks_declarations::visit_leave(ir_assignment *ir)
>        }
>  
>        ir_variable *lhs_var =  lhs_rec_tmp->variable_referenced();
>        if (lhs_var) {
>           lhs_var->data.assigned = 1;
>        }
>     }
>     return rvalue_visit(ir);
>  }
>  
> +ir_visitor_status
> +flatten_named_interface_blocks_declarations::visit_leave(ir_expression *ir)
> +{
> +   ir_visitor_status status = rvalue_visit(ir);
> +
> +   if (ir->operation == ir_unop_interpolate_at_centroid ||
> +       ir->operation == ir_binop_interpolate_at_offset ||
> +       ir->operation == ir_binop_interpolate_at_sample) {
> +      const ir_rvalue *val = ir->operands[0];
> +
> +      /* This disables varying packing for this input. */
> +      val->variable_referenced()->data.must_be_shader_input = 1;
> +   }
> +
> +   return status;
> +}
> +
>  void
>  flatten_named_interface_blocks_declarations::handle_rvalue(ir_rvalue **rvalue)
>  {
>     if (*rvalue == NULL)
>        return;
>  
>     ir_dereference_record *ir = (*rvalue)->as_dereference_record();
>     if (ir == NULL)
>        return;
>  
>