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

[Iago Toral]

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.

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

> +      .lower_vote_trivial = !is_scalar,
> +   };
> +   OPT(nir_lower_subgroups, &subgroups_options);
> +
>     OPT(nir_lower_clip_cull_distance_arrays);
>  
>     nir_variable_mode indirect_mask = 0;