On Mon, 2017-10-30 at 11:45 -0700, Jason Ekstrand wrote:
> On Mon, Oct 30, 2017 at 4:38 AM, Iago Toral <ito...@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.
oh, ok. Yeah, I suppose that in that case moving it there would make
more sense in the long run, but that's something we can do some other
time.
>
> > > 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.
Ah, right. Thanks for explaining.
> > > + .lower_vote_trivial = !is_scalar,
> >
> > > + };
> >
> > > + OPT(nir_lower_subgroups, &subgroups_options);
> >
> > > +
> >
> > > OPT(nir_lower_clip_cull_distance_arrays);
> >
> > >
> >
> > > nir_variable_mode indirect_mask = 0;
> >
> > _______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
