[Mesa-dev] [PATCH 3/8] nir: add a helper for getting the bitmask for a variable's location

[Timothy Arceri]

On 23/09/17 04:33, Kenneth Graunke wrote:
> On Tuesday, September 12, 2017 4:37:30 PM PDT Timothy Arceri wrote:
>> ---
>>   src/compiler/nir/nir.h | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>> index fab2110f619..e52a1006896 100644
>> --- a/src/compiler/nir/nir.h
>> +++ b/src/compiler/nir/nir.h
>> @@ -351,6 +351,37 @@ typedef struct nir_variable {
>>   #define nir_foreach_variable_safe(var, var_list) \
>>      foreach_list_typed_safe(nir_variable, var, node, var_list)
>>   
>> +/**
>> + * Returns the bits in the inputs_read, outputs_written, or
>> + * system_values_read bitfield corresponding to this variable.
>> + */
>> +static inline uint64_t
>> +nir_variable_get_io_mask(nir_variable *var, gl_shader_stage stage)
>> +{
>> +   /* TODO: add support for tess patches */
>> +   if (var->data.patch || var->data.location < 0)
>> +      return 0;
>> +
>> +   assert(var->data.mode == nir_var_shader_in ||
>> +          var->data.mode == nir_var_shader_out ||
>> +          var->data.mode == nir_var_system_value);
>> +   assert(var->data.location >= 0);
>> +
>> +   const struct glsl_type *var_type = var->type;
>> +   if ((var->data.mode == nir_var_shader_in &&
>> +        (stage == MESA_SHADER_GEOMETRY ||
>> +         stage == MESA_SHADER_TESS_CTRL ||
>> +         stage == MESA_SHADER_TESS_EVAL)) ||
>> +       (var->data.mode == nir_var_shader_out &&
>> +        stage == MESA_SHADER_TESS_CTRL)) {
>> +      if (glsl_type_is_array(var_type))
>> +         var_type = glsl_get_array_element(var_type);
> 
> You can just do
> 
>     if (nir_is_per_vertex_io(var, stage)) {
>        assert(glsl_type_is_array(type));
>        type = glsl_get_array_element(type);
>     }

Right, thanks :)

> 
>> +   }
>> +
>> +   unsigned slots = glsl_count_attribute_slots(var_type, false);
>> +   return ((1ull << slots) - 1) << var->data.location;
>> +}
>> +
>>   static inline bool
>>   nir_variable_is_global(const nir_variable *var)
>>   {
>>
> 
> This is duplicating the code in nir_gather_info.c pretty hard, but it
> doesn't handle patch variables, or scalar arrays with var->data.compact
> set (i.e. gl_ClipDistance as float[8] packed as if it were vec4[2]).

We don't try to remove builtins so var->data.compact shouldn't be a 
problem right?

> 
> Would it make sense to wrap or refactor set_io_mask()?
> 

Maybe, I'm not sure. There is a bunch of stuff in nir_gather_info.c we 
don't really need just yet. For example we currently make no attempt to 
remove components from partially used arrays.

I'd really like to land this first, which with your suggestion above is 
really a very small amount of code. From here I'm working on a NIR 
vector splitting pass and small update to this series to handle removal 
per component rather than per location. Initial results (take with a 
gain of salt as I still have a handful of piglit issues) are looking 
very promising.

total instructions in shared programs: 13190730 -> 13110599 (-0.61%)
instructions in affected programs: 2110903 -> 2030772 (-3.80%)
helped: 14043
HURT: 486

total cycles in shared programs: 541148990 -> 540561942 (-0.11%)
cycles in affected programs: 290344296 -> 289757248 (-0.20%)
helped: 23418
HURT: 11623

Once I write the NIR packing pass this should get even better.

I'd really like it if we could just land this patch as is then come back 
and improve things once a more full linking implementation is in place 
rather than trying to get it perfect in one step. Does that seem reasonable?