[Mesa-dev] [PATCH 8/8] i965: make use of nir linking

[Timothy Arceri]

On 26/09/17 08:47, Kenneth Graunke wrote:
> On Tuesday, September 12, 2017 4:37:35 PM PDT Timothy Arceri wrote:
>> For now linking is just removing unused varyings between stages.
>>
>> shader-db results BDW:
>>
>> total instructions in shared programs: 13198288 -> 13191693 (-0.05%)
>> instructions in affected programs: 48325 -> 41730 (-13.65%)
>> helped: 473
>> HURT: 0
>>
>> total cycles in shared programs: 541184926 -> 541159260 (-0.00%)
>> cycles in affected programs: 213238 -> 187572 (-12.04%)
>> helped: 435
>> HURT: 8
>> ---
>>   src/mesa/drivers/dri/i965/brw_link.cpp | 48 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp b/src/mesa/drivers/dri/i965/brw_link.cpp
>> index 9f1634a5459..c4c89a0686e 100644
>> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
>> @@ -252,6 +252,54 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg)
>>                                    compiler->scalar_stage[stage]);
>>      }
>>   
>> +   /* Determine first and last stage. */
>> +   unsigned first = MESA_SHADER_STAGES;
>> +   unsigned last = 0;
>> +   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>> +      if (!shProg->_LinkedShaders[i])
>> +         continue;
>> +      if (first == MESA_SHADER_STAGES)
>> +         first = i;
>> +      last = i;
>> +   }
>> +
>> +   /* Linking the stages in the opposite order (from fragment to vertex)
>> +    * ensures that inter-shader outputs written to in an earlier stage
>> +    * are eliminated if they are (transitively) not used in a later
>> +    * stage.
>> +    */
>> +    if (first != last) {
>> +       int next = last;
>> +       for (int i = next - 1; i >= 0; i--) {
>> +          if (shProg->_LinkedShaders[i] == NULL)
>> +             continue;
>> +
>> +            nir_shader *producer = shProg->_LinkedShaders[i]->Program->nir;
>> +            nir_shader *consumer = shProg->_LinkedShaders[next]->Program->nir;
>> +
>> +            nir_remove_dead_variables(shProg->_LinkedShaders[next]->Program->nir,
>> +                                      nir_var_shader_in);
>> +            nir_remove_dead_variables(shProg->_LinkedShaders[i]->Program->nir,
>> +                                      nir_var_shader_out);
>> +            if (nir_remove_unused_varyings(producer, consumer)) {
>> +               nir_lower_global_vars_to_local(producer);
>> +               nir_lower_global_vars_to_local(consumer);
>> +
>> +               nir_variable_mode indirect_mask = (nir_variable_mode) 0;
>> +               if (compiler->glsl_compiler_options[i].EmitNoIndirectTemp)
>> +                  indirect_mask = (nir_variable_mode) nir_var_local;
>> +
>> +               nir_lower_indirect_derefs(producer, indirect_mask);
> 
> Ugh, good catch...by demoting inputs/outputs to globals, we might end up
> with indirects on temporaries, so we do need to lower them here.  (It's
> a bit annoying that we don't handle indirects on temporaries in the FS
> backend in 2017...I asked krh to implement that in review feedback for
> his 2014-era SIMD8 VS series, and then jekstrand actually implemented it
> in 2015, and it never landed for some reason...but...I digress...)
> 
>> +
>> +               const bool is_scalar = compiler->scalar_stage[producer->stage];
>> +               shProg->_LinkedShaders[i]->Program->nir =
>> +                 brw_nir_optimize(producer, compiler, is_scalar);
> 
> I'm a little lost why you only lower indirect derefs and optimize for
> the producer stage.  nir_remove_unused_varyings can demote variables in
> either stage.  For example, if the consumer reads a variable, but the
> producer doesn't write it, then we'll have a consumer-only variable get
> demoted.
> 
> It seems like the consumer is the more important one, too, because
> you're walking backwards, so the current producer may get hit by the
> next pass...unless it's the first stage...so...don't we need to do both?

Yeah I think your right.

> 
>> +            }
>> +
>> +            next = i;
>> +       }
>> +    }
>> +
>>      for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders); stage++) {
>>         struct gl_linked_shader *shader = shProg->_LinkedShaders[stage];
>>         if (!shader)
>>
>