<html><head></head><body><div>On Mon, 2017-10-30 at 11:45 -0700, Jason Ekstrand wrote:</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 30, 2017 at 4:38 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite"><span class="">On Wed, 2017-10-25 at 16:26 -0700, Jason Ekstrand wrote:<br>
> This commit pulls nir_lower_read_invocations_to_<wbr>scalar along with<br>
> most<br>
> of the guts of nir_opt_intrinsics (which mostly does subgroup<br>
> lowering)<br>
> into a new nir_lower_subgroups pass. There are various other bits of<br>
> subgroup lowering that we're going to want to do so it makes a bit<br>
> more<br>
> sense to keep it all together in one pass. We also move it in i965<br>
> to<br>
> happen after nir_lower_system_values to ensure that because we want<br>
> to<br>
> handle the subgroup mask system value intrinsics here.<br>
> ---<br>
> src/compiler/Makefile.<wbr>sources |<wbr> 2 +-<br>
> src/compiler/nir/nir.h <wbr> | 12 +-<br>
> .../nir/nir_lower_read_<wbr>invocation_to_scalar.c | 112 ---------<br>
> -----<br>
</span>(...)<br>
<span class="">> diff --git a/src/compiler/nir/nir_opt_<wbr>intrinsics.c<br>
> b/src/compiler/nir/nir_opt_<wbr>intrinsics.c<br>
> index 26a0f96..98c8b1a 100644<br>
> --- a/src/compiler/nir/nir_opt_<wbr>intrinsics.c<br>
> +++ b/src/compiler/nir/nir_opt_<wbr>intrinsics.c<br>
> @@ -46,22 +46,14 @@ opt_intrinsics_impl(nir_<wbr>function_impl *impl)<br>
> <br>
> switch (intrin->intrinsic) {<br>
> case nir_intrinsic_vote_any:<br>
> - case nir_intrinsic_vote_all: {<br>
> - nir_const_value *val = nir_src_as_const_value(intrin-<br>
> >src[0]);<br>
> - if (!val && !b.shader->options->lower_<wbr>vote_trivial)<br>
> - continue;<br>
> -<br>
> - replacement = nir_ssa_for_src(&b, intrin->src[0], 1);<br>
> + case nir_intrinsic_vote_all:<br>
> + if (nir_src_as_const_value(<wbr>intrin->src[0]))<br>
> + replacement = nir_ssa_for_src(&b, intrin->src[0], 1);<br>
<br>
</span>Isn't this redundant with what is being done in nir_lower_subgroups.c?<br>
I was expectin that this code here would all be removed.<span class=""><br></span><br></blockquote><div><br></div><div>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.<br></div></div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote type="cite"><span class="">
> break;<br>
> - }<br>
> - case nir_intrinsic_vote_eq: {<br>
> - nir_const_value *val = nir_src_as_const_value(intrin-<br>
> >src[0]);<br>
> - if (!val && !b.shader->options->lower_<wbr>vote_trivial)<br>
> - continue;<br>
> -<br>
> - replacement = nir_imm_int(&b, NIR_TRUE);<br>
> + case nir_intrinsic_vote_eq:<br>
> + if (nir_src_as_const_value(<wbr>intrin->src[0]))<br>
> + replacement = nir_imm_int(&b, NIR_TRUE);<br>
<br>
</span>Same here.<br>
<br>
> break;<br>
> - }<br>
<br>
(...)<br>
<div><div class="h5"><br>
> diff --git a/src/intel/compiler/brw_<wbr>compiler.c<br>
> b/src/intel/compiler/brw_<wbr>compiler.c<br>
> index 2f6af7d..a6129e9 100644<br>
> --- a/src/intel/compiler/brw_<wbr>compiler.c<br>
> +++ b/src/intel/compiler/brw_<wbr>compiler.c<br>
> @@ -57,7 +57,6 @@ static const struct nir_shader_compiler_options<br>
> scalar_nir_options = {<br>
> .lower_unpack_snorm_4x8 = true,<br>
> .lower_unpack_unorm_2x16 = true,<br>
> .lower_unpack_unorm_4x8 = true,<br>
> - .lower_subgroup_masks = true,<br>
> .max_subgroup_size = 32,<br>
> .max_unroll_iterations = 32,<br>
> };<br>
> @@ -80,7 +79,6 @@ static const struct nir_shader_compiler_options<br>
> vector_nir_options = {<br>
> .lower_unpack_unorm_2x16 = true,<br>
> .lower_extract_byte = true,<br>
> .lower_extract_word = true,<br>
> - .lower_vote_trivial = true,<br>
> .max_unroll_iterations = 32,<br>
> };<br>
> <br>
> @@ -99,7 +97,6 @@ static const struct nir_shader_compiler_options<br>
> vector_nir_options_gen6 = {<br>
> .lower_unpack_unorm_2x16 = true,<br>
> .lower_extract_byte = true,<br>
> .lower_extract_word = true,<br>
> - .lower_vote_trivial = true,<br>
> .max_unroll_iterations = 32,<br>
> };<br>
> <br>
> diff --git a/src/intel/compiler/brw_nir.c<br>
> b/src/intel/compiler/brw_nir.c<br>
> index e5ff6de..f599f74 100644<br>
> --- a/src/intel/compiler/brw_nir.c<br>
> +++ b/src/intel/compiler/brw_nir.c<br>
> @@ -620,7 +620,6 @@ brw_preprocess_nir(const struct brw_compiler<br>
> *compiler, nir_shader *nir)<br>
> <br>
> OPT(nir_lower_tex, &tex_options);<br>
> OPT(nir_normalize_cubemap_<wbr>coords);<br>
> - OPT(nir_lower_read_<wbr>invocation_to_scalar);<br>
> <br>
> OPT(nir_lower_global_vars_<wbr>to_local);<br>
> <br>
> @@ -637,6 +636,13 @@ brw_preprocess_nir(const struct brw_compiler<br>
> *compiler, nir_shader *nir)<br>
> <br>
> OPT(nir_lower_system_<wbr>values);<br>
> <br>
> + const nir_lower_subgroups_options subgroups_options = {<br>
> + .lower_to_scalar = true,<br>
> + .lower_subgroup_masks = true,<br>
<br>
</div></div>lower_subgroup_masks was not being set for vector compiles before, so<br>
this is a change. Is this intended?<br><div class="HOEnZb"><div class="h5"></div></div><br></blockquote><div><br></div><div>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.<br></div></div></div></div></blockquote><div><br></div><div>Ah, right. Thanks for explaining.</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div class="HOEnZb"><div class="h5">
> + .lower_vote_trivial = !is_scalar,<br>
> + };<br>
> + OPT(nir_lower_subgroups, &subgroups_options);<br>
> +<br>
> OPT(nir_lower_clip_cull_<wbr>distance_arrays);<br>
> <br>
> nir_variable_mode indirect_mask = 0;<br>
</div></div><br></blockquote></div><br></div></div>
</blockquote></body></html>