[Mesa-dev] [PATCH 1/2] nir: add new nir_remove_dead_barriers() pass

[Timothy Arceri]

On 14/11/18 11:39 pm, Bas Nieuwenhuizen wrote:
> On Wed, Nov 14, 2018 at 1:03 PM Timothy Arceri <tarceri at itsqueeze.com> wrote:
>>
>> On 14/11/18 10:22 pm, Bas Nieuwenhuizen wrote:
>>> On Wed, Nov 14, 2018 at 6:30 AM Timothy Arceri <tarceri at itsqueeze.com> wrote:
>>>>
>>>> Link time opts can remove unused outputs leaving behind useless
>>>> barriers. This pass is intended to be called after linking so
>>>> I've added it to nir_linking_helpers.c
>>>>
>>>> This pass removes some barriers from Witcher 3 (DXVK) Vulkan
>>>> shaders.
>>>> ---
>>>>    src/compiler/nir/nir.h                 |  1 +
>>>>    src/compiler/nir/nir_linking_helpers.c | 38 ++++++++++++++++++++++++++
>>>>    2 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>>>> index c469e111b2..224d2f66e2 100644
>>>> --- a/src/compiler/nir/nir.h
>>>> +++ b/src/compiler/nir/nir.h
>>>> @@ -2817,6 +2817,7 @@ void nir_compact_varyings(nir_shader *producer, nir_shader *consumer,
>>>>                              bool default_to_smooth_interp);
>>>>    void nir_link_xfb_varyings(nir_shader *producer, nir_shader *consumer);
>>>>    bool nir_link_constant_varyings(nir_shader *producer, nir_shader *consumer);
>>>> +void nir_remove_dead_barriers(nir_shader *shader);
>>>>
>>>>    typedef enum {
>>>>       /* If set, this forces all non-flat fragment shader inputs to be
>>>> diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
>>>> index a05890ada4..61982659a0 100644
>>>> --- a/src/compiler/nir/nir_linking_helpers.c
>>>> +++ b/src/compiler/nir/nir_linking_helpers.c
>>>> @@ -669,3 +669,41 @@ nir_link_constant_varyings(nir_shader *producer, nir_shader *consumer)
>>>>
>>>>       return progress;
>>>>    }
>>>> +/* This is intended to be called this after linking opts as unused tcs outputs
>>>> + * may have been removed leaving useless barriers in the shader.
>>>> + *
>>>> + * TODO: We could make this pass more complex and actually check if we are
>>>> + * reading outputs set by other invocations.
>>>> + */
>>>> +void
>>>> +nir_remove_dead_barriers(nir_shader *shader)
>>>> +{
>>>> +   if (shader->info.stage != MESA_SHADER_TESS_CTRL)
>>>> +      return;
>>>> +
>>>> +   bool progress = false;
>>>> +
>>>> +   nir_function_impl *impl = nir_shader_get_entrypoint(shader);
>>>> +
>>>> +   nir_foreach_block(block, impl) {
>>>> +      nir_foreach_instr_safe(instr, block) {
>>>> +         if (instr->type != nir_instr_type_intrinsic)
>>>> +            continue;
>>>> +
>>>> +         nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
>>>> +
>>>> +         /* If we see a store or load of an output then bail */
>>>> +         if (intr->intrinsic == nir_intrinsic_store_deref ||
>>>> +             intr->intrinsic == nir_intrinsic_load_deref) {
>>>> +            nir_variable *var =
>>>> +               nir_deref_instr_get_variable(nir_src_as_deref(intr->src[0]));
>>>> +            if (var->data.mode == nir_var_shader_out)
>>>> +               return;
>>>> +         }
>>>> +
>>>> +         /* Remove barriers if we have not yet touched any outputs */
>>>> +         if (intr->intrinsic == nir_intrinsic_barrier)
>>>> +            nir_instr_remove(instr);
>>>> +      }
>>>> +   }
>>>
>>> So we are going through blocks first to last right. Does this work
>>> with loops? e.g. if we have something like
>>>
>>> {
>>>     for (...) {
>>>       barrier();
>>>       touch outputs
>>>     }
>>> }
>>>
>>> In this case I'm not sure it is safe to remove the barrier but we might do so?
>>
>> At least for TCS that is not possible:
>>
>> "The barrier() function may only be called inside the main entry point
>> of the tessellation control shader and may not be called in potentially
>> divergent flow control.  In particular, barrier() may not be called
>> inside a switch statement, in either sub-statement of an if statement,
>> inside a do, for, or while loop, or at any point after a return
>> statement in the function main()."
> 
> I don't think this restriction holds for vulkan/spir-v:
> 
> https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#_a_id_barrier_a_barrier_instructions
> 
> and
> 
> " If OpControlBarrier is used in fragment, vertex, tessellation
> evaluation, or geometry stages, the
> execution Scope must be Subgroup." (vulkan spec, Appendix A)
> 
> say nothing about happening in control flow. It just needs to be in
> dynamically uniform control flow.

Ok, thanks. I didn't realize Subgroups used the same op this make 
removing barriers more complicated. I think I'll just leave this alone 
for now.

> 
>>
>> Compute shaders are different but this function only supports TCS for
>> now. The only reason we look at blocks in control flow is we still need
>> to bail if we see a load/store on an out.
>>
>>>
>>>> +}
>>>> --
>>>> 2.19.1
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev