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

[Nicolai Hähnle]

On 18.06.2017 21:18, Ian Romanick wrote:
> 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 sent a series that adds more tests.


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

Sure.


> 
>> ---
>>   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.

Makes sense, changed locally.

Cheers,
Nicolai

> 
>>         }
>>   
>>         /* 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;
>>   
>>
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.