[Mesa-dev] [PATCH v3 39/48] nir: Add a new subgroups lowering pass

[Jason Ekstrand]

On Mon, Oct 30, 2017 at 4:38 AM, Iago Toral <itoral at igalia.com> wrote:

> On Wed, 2017-10-25 at 16:26 -0700, Jason Ekstrand wrote:
> > This commit pulls nir_lower_read_invocations_to_scalar along with
> > most
> > of the guts of nir_opt_intrinsics (which mostly does subgroup
> > lowering)
> > into a new nir_lower_subgroups pass.  There are various other bits of
> > subgroup lowering that we're going to want to do so it makes a bit
> > more
> > sense to keep it all together in one pass.  We also move it in i965
> > to
> > happen after nir_lower_system_values to ensure that because we want
> > to
> > handle the subgroup mask system value intrinsics here.
> > ---
> >  src/compiler/Makefile.sources                      |   2 +-
> >  src/compiler/nir/nir.h                             |  12 +-
> >  .../nir/nir_lower_read_invocation_to_scalar.c      | 112 ---------
> > -----
> (...)
> > diff --git a/src/compiler/nir/nir_opt_intrinsics.c
> > b/src/compiler/nir/nir_opt_intrinsics.c
> > index 26a0f96..98c8b1a 100644
> > --- a/src/compiler/nir/nir_opt_intrinsics.c
> > +++ b/src/compiler/nir/nir_opt_intrinsics.c
> > @@ -46,22 +46,14 @@ opt_intrinsics_impl(nir_function_impl *impl)
> >
> >           switch (intrin->intrinsic) {
> >           case nir_intrinsic_vote_any:
> > -         case nir_intrinsic_vote_all: {
> > -            nir_const_value *val = nir_src_as_const_value(intrin-
> > >src[0]);
> > -            if (!val && !b.shader->options->lower_vote_trivial)
> > -               continue;
> > -
> > -            replacement = nir_ssa_for_src(&b, intrin->src[0], 1);
> > +         case nir_intrinsic_vote_all:
> > +            if (nir_src_as_const_value(intrin->src[0]))
> > +               replacement = nir_ssa_for_src(&b, intrin->src[0], 1);
>
> Isn't this redundant with what is being done in nir_lower_subgroups.c?
> I was expectin that this code here would all be removed.
>

The old code was handling two cases:  trivial lowering and constant
folding.  The former is now part of lowering but the later is an
optimization that needs to be run as part of the optimization loop.  One
could make a case that these could be rolled into nir_opt_constant_folding.


> >              break;
> > -         }
> > -         case nir_intrinsic_vote_eq: {
> > -            nir_const_value *val = nir_src_as_const_value(intrin-
> > >src[0]);
> > -            if (!val && !b.shader->options->lower_vote_trivial)
> > -               continue;
> > -
> > -            replacement = nir_imm_int(&b, NIR_TRUE);
> > +         case nir_intrinsic_vote_eq:
> > +            if (nir_src_as_const_value(intrin->src[0]))
> > +               replacement = nir_imm_int(&b, NIR_TRUE);
>
> Same here.
>
> >              break;
> > -         }
>
> (...)
>
> > diff --git a/src/intel/compiler/brw_compiler.c
> > b/src/intel/compiler/brw_compiler.c
> > index 2f6af7d..a6129e9 100644
> > --- a/src/intel/compiler/brw_compiler.c
> > +++ b/src/intel/compiler/brw_compiler.c
> > @@ -57,7 +57,6 @@ static const struct nir_shader_compiler_options
> > scalar_nir_options = {
> >     .lower_unpack_snorm_4x8 = true,
> >     .lower_unpack_unorm_2x16 = true,
> >     .lower_unpack_unorm_4x8 = true,
> > -   .lower_subgroup_masks = true,
> >     .max_subgroup_size = 32,
> >     .max_unroll_iterations = 32,
> >  };
> > @@ -80,7 +79,6 @@ static const struct nir_shader_compiler_options
> > vector_nir_options = {
> >     .lower_unpack_unorm_2x16 = true,
> >     .lower_extract_byte = true,
> >     .lower_extract_word = true,
> > -   .lower_vote_trivial = true,
> >     .max_unroll_iterations = 32,
> >  };
> >
> > @@ -99,7 +97,6 @@ static const struct nir_shader_compiler_options
> > vector_nir_options_gen6 = {
> >     .lower_unpack_unorm_2x16 = true,
> >     .lower_extract_byte = true,
> >     .lower_extract_word = true,
> > -   .lower_vote_trivial = true,
> >     .max_unroll_iterations = 32,
> >  };
> >
> > diff --git a/src/intel/compiler/brw_nir.c
> > b/src/intel/compiler/brw_nir.c
> > index e5ff6de..f599f74 100644
> > --- a/src/intel/compiler/brw_nir.c
> > +++ b/src/intel/compiler/brw_nir.c
> > @@ -620,7 +620,6 @@ brw_preprocess_nir(const struct brw_compiler
> > *compiler, nir_shader *nir)
> >
> >     OPT(nir_lower_tex, &tex_options);
> >     OPT(nir_normalize_cubemap_coords);
> > -   OPT(nir_lower_read_invocation_to_scalar);
> >
> >     OPT(nir_lower_global_vars_to_local);
> >
> > @@ -637,6 +636,13 @@ brw_preprocess_nir(const struct brw_compiler
> > *compiler, nir_shader *nir)
> >
> >     OPT(nir_lower_system_values);
> >
> > +   const nir_lower_subgroups_options subgroups_options = {
> > +      .lower_to_scalar = true,
> > +      .lower_subgroup_masks = true,
>
> lower_subgroup_masks was not being set for vector compiles before, so
> this is a change. Is this intended?
>

Yes and no.  Matt didn't set it on vec4 shaders because we haven't enabled
GL_ARB_shader_ballot on anything earlier than gen8 because it relies on
int64.  Even with the Vulkan feature, we won't support the geometry
pipeline on gen7 so they'll never be seen.  It doesn't do us any good to
set it but it also doesn't hurt.


> > +      .lower_vote_trivial = !is_scalar,
> > +   };
> > +   OPT(nir_lower_subgroups, &subgroups_options);
> > +
> >     OPT(nir_lower_clip_cull_distance_arrays);
> >
> >     nir_variable_mode indirect_mask = 0;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171030/1ae76942/attachment-0001.html>