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

[Bas Nieuwenhuizen]

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.

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