<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Aug 23, 2018 at 5:42 PM Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sat, Jul 28, 2018 at 10:44:42PM -0700, Jason Ekstrand wrote:<br>
> We have to be a bit careful with this one because we want it to run in<br>
> the optimization loop but only in the first brw_nir_optimize call.<br>
> Later calls assume that we've lowered away copy_deref instructions and<br>
> we don't want to introduce any more.<br>
> <br>
> Shader-db results on Kaby Lake:<br>
> <br>
> total instructions in shared programs: 15176942 -> 15176942 (0.00%)<br>
> instructions in affected programs: 0 -> 0<br>
> helped: 0<br>
> HURT: 0<br>
> <br>
> In spite of the lack of any shader-db improvement, this patch completely<br>
> eliminates spilling in the Batman: Arkham City tessellation shaders.<br>
> This is because we are now able to detect that the temporary array<br>
> created by DXVK for storing TCS inputs is a copy of the input arrays and<br>
> use indirect URB reads instead of making a copy of 4.5 KiB of input data<br>
> and then indirecting on it with if-ladders.<br>
> ---<br>
> src/intel/compiler/brw_nir.c | 16 +++++++++-------<br>
> src/intel/compiler/brw_nir.h | 3 ++-<br>
> 2 files changed, 11 insertions(+), 8 deletions(-)<br>
<br>
Given the comment mentioned below is added. This patch is<br>
<br>
Reviewed-by: Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com" target="_blank">caio.oliveira@intel.com</a>><br>
<br>
There's also an optional suggestion.<br>
<br>
<br>
<br>
> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c<br>
> index 5e9da9e1ef2..2417c0cd618 100644<br>
> --- a/src/intel/compiler/brw_nir.c<br>
> +++ b/src/intel/compiler/brw_nir.c<br>
> @@ -533,7 +533,7 @@ brw_nir_no_indirect_mask(const struct brw_compiler *compiler,<br>
> <br>
> nir_shader *<br>
> brw_nir_optimize(nir_shader *nir, const struct brw_compiler *compiler,<br>
> - bool is_scalar)<br>
> + bool is_scalar, bool allow_copies)<br>
> {<br>
> nir_variable_mode indirect_mask =<br>
> brw_nir_no_indirect_mask(compiler, nir->info.stage);<br>
> @@ -544,6 +544,8 @@ brw_nir_optimize(nir_shader *nir, const struct brw_compiler *compiler,<br>
> OPT(nir_split_array_vars, nir_var_local);<br>
> OPT(nir_shrink_vec_array_vars, nir_var_local);<br>
> OPT(nir_lower_vars_to_ssa);<br>
> + if (allow_copies)<br>
> + OPT(nir_opt_find_array_copies);<br>
> OPT(nir_opt_copy_prop_vars);<br>
> <br>
> if (is_scalar) {<br>
<br>
Adding a comment here with a subset of your first paragraph in the commit<br>
message will make reduce the chance of future changes to optimize mess<br>
things up<br></blockquote><div><br></div><div>Done.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> @@ -664,7 +666,7 @@ brw_preprocess_nir(const struct brw_compiler *compiler, nir_shader *nir)<br>
> nir_lower_isign64 |<br>
> nir_lower_divmod64);<br>
> <br>
> - nir = brw_nir_optimize(nir, compiler, is_scalar);<br>
> + nir = brw_nir_optimize(nir, compiler, is_scalar, true);<br>
> <br>
> /* This needs to be run after the first optimization pass but before we<br>
> * lower indirect derefs away<br>
> @@ -701,7 +703,7 @@ brw_preprocess_nir(const struct brw_compiler *compiler, nir_shader *nir)<br>
> nir_lower_indirect_derefs(nir, indirect_mask);<br>
> <br>
> /* Get rid of split copies */<br>
> - nir = brw_nir_optimize(nir, compiler, is_scalar);<br>
> + nir = brw_nir_optimize(nir, compiler, is_scalar, false);<br>
<br>
Optional: the call-sites usually have a "is_scalar" variable around,<br>
making that boolean parameter "self-documenting". This is not the<br>
case for allow_copies. One idea is to make this last argument a flag,<br>
and add an enum for the ALLOW_CREATE_DEREF_COPIES or something like.<br></blockquote><div><br></div><div>I thought seriously about doing that the first time around. Maybe I'll cook up a follow-on patch.</div><div><br></div><div>--Jason<br></div></div></div>