<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Dec 21, 2016 at 6:33 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div><div class="m_6473124787206035435gmail-h5"><div><div class="gmail_extra"><div class="gmail_quote">On Dec 21, 2016 8:11 PM, "Timothy Arceri" <<a href="mailto:timothy.arceri@collabora.com" target="_blank">timothy.arceri@collabora.com</a>> wrote:<br type="attribution"><blockquote class="m_6473124787206035435gmail-m_7302714870370531757quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_6473124787206035435gmail-m_7302714870370531757elided-text">On Mon, 2016-12-19 at 20:11 -0800, Jason Ekstrand wrote:<br>
> ---<br>
> src/compiler/Makefile.sources<wbr> | 1 +<br>
> src/compiler/nir/nir.h <wbr> | 2 +<br>
> src/compiler/nir/nir_opt_triv<wbr>ial_continues.c | 141<br>
> +++++++++++++++++++++++++++<br>
> 3 files changed, 144 insertions(+)<br>
> create mode 100644 src/compiler/nir/nir_opt_trivi<wbr>al_continues.c<br>
><br>
> diff --git a/src/compiler/Makefile.source<wbr>s<br>
> b/src/compiler/Makefile.source<wbr>s<br>
> index ae3e5f0..a0abede 100644<br>
> --- a/src/compiler/Makefile.source<wbr>s<br>
> +++ b/src/compiler/Makefile.source<wbr>s<br>
> @@ -242,6 +242,7 @@ NIR_FILES = \<br>
> nir/nir_opt_loop_unroll.c \<br>
> nir/nir_opt_peephole_select.c \<br>
> nir/nir_opt_remove_phis.c \<br>
> + nir/nir_opt_trivial_continues<wbr>.c \<br>
> nir/nir_opt_undef.c \<br>
> nir/nir_phi_builder.c \<br>
> nir/nir_phi_builder.h \<br>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h<br>
> index 89b0c70..a6c8956 100644<br>
> --- a/src/compiler/nir/nir.h<br>
> +++ b/src/compiler/nir/nir.h<br>
> @@ -2563,6 +2563,8 @@ bool nir_opt_peephole_select(nir_sh<wbr>ader<br>
> *shader, unsigned limit);<br>
> <br>
> bool nir_opt_remove_phis(nir_shader *shader);<br>
> <br>
> +bool nir_opt_trivial_continues(nir_<wbr>shader *shader);<br>
> +<br>
> bool nir_opt_undef(nir_shader *shader);<br>
> <br>
> bool nir_opt_conditional_discard(ni<wbr>r_shader *shader);<br>
> diff --git a/src/compiler/nir/nir_opt_tri<wbr>vial_continues.c<br>
> b/src/compiler/nir/nir_opt_tri<wbr>vial_continues.c<br>
> new file mode 100644<br>
> index 0000000..ff3205d<br>
> --- /dev/null<br>
> +++ b/src/compiler/nir/nir_opt_tri<wbr>vial_continues.c<br>
> @@ -0,0 +1,141 @@<br>
> +/*<br>
> + * Copyright © 2016 Intel Corporation<br>
> + *<br>
> + * Permission is hereby granted, free of charge, to any person<br>
> obtaining a<br>
> + * copy of this software and associated documentation files (the<br>
> "Software"),<br>
> + * to deal in the Software without restriction, including without<br>
> limitation<br>
> + * the rights to use, copy, modify, merge, publish, distribute,<br>
> sublicense,<br>
> + * and/or sell copies of the Software, and to permit persons to whom<br>
> the<br>
> + * Software is furnished to do so, subject to the following<br>
> conditions:<br>
> + *<br>
> + * The above copyright notice and this permission notice (including<br>
> the next<br>
> + * paragraph) shall be included in all copies or substantial<br>
> portions of the<br>
> + * Software.<br>
> + *<br>
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,<br>
> EXPRESS OR<br>
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF<br>
> MERCHANTABILITY,<br>
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO<br>
> EVENT SHALL<br>
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES<br>
> OR OTHER<br>
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,<br>
> ARISING<br>
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR<br>
> OTHER DEALINGS<br>
> + * IN THE SOFTWARE.<br>
> + */<br>
> +<br>
> +#include "nir.h"<br>
> +<br>
> +static bool<br>
> +instr_is_continue(nir_instr *instr)<br>
> +{<br>
> + if (instr->type != nir_instr_type_jump)<br>
> + return false;<br>
> +<br>
> + return nir_instr_as_jump(instr)->type == nir_jump_continue;<br>
> +}<br>
> +<br>
> +static bool<br>
> +lower_trivial_continues_block<wbr>(nir_block *block, nir_loop *loop)<br>
> +{<br>
> + bool progress = false;<br>
> + nir_instr *first_instr = nir_block_first_instr(block);<br>
> + if (!first_instr || instr_is_continue(first_instr)<wbr>) {<br>
> + /* The block contains only a continue if anything */<br>
> + nir_cf_node *prev_node = nir_cf_node_prev(&block->cf_no<wbr>de);<br>
> + if (prev_node && prev_node->type == nir_cf_node_if) {<br>
> + nir_if *prev_if = nir_cf_node_as_if(prev_node);<br>
> + progress |= lower_trivial_continues_block(<br>
> + nir_if_last_then_<wbr>block(prev_if), loop);<br>
> + progress |= lower_trivial_continues_block(<br>
> + nir_if_last_else_<wbr>block(prev_if), loop);<br>
> + }<br>
> +<br>
> + /* The lower_phis_to_regs helper is smart enough to push phi<br>
> sources as<br>
> + * far up if-ladders as it possibly can. In other words, the<br>
> recursive<br>
> + * calls above shouldn't be adding instructions to this block<br>
> since it<br>
> + * follows an if statement.<br>
> + */<br>
> + first_instr = nir_block_first_instr(block);<br>
> + assert(!first_instr || instr_is_continue(first_instr)<wbr>);<br>
> + }<br>
> +<br>
> + nir_instr *last_instr = nir_block_last_instr(block);<br>
> + if (!last_instr || !instr_is_continue(last_instr)<wbr>)<br>
> + return progress;<br>
> +<br>
> + /* We're at the end of a block that goes straight through to the<br>
> end of the<br>
> + * loop and continues to the top. We can exit normally instead<br>
> of jump.<br>
> + */<br>
> + nir_lower_phis_to_regs_blo<wbr>ck(nir_loop_first_block(loop))<wbr>;<br>
> + nir_instr_remove(last_inst<wbr>r);<br>
> + return true;<br>
> +}<br>
> +<br>
> +static bool<br>
> +lower_trivial_continues_list(<wbr>struct exec_list *cf_list,<br>
> + <wbr>bool list_ends_at_loop_tail,<br>
> + <wbr>nir_loop *loop)<br>
> +{<br>
> + bool progress = false;<br>
> + foreach_list_typed(nir_cf_<wbr>node, cf_node, node, cf_list) {<br>
> + bool at_loop_tail = list_ends_at_loop_tail &&<br>
> + &cf<wbr>_node->node ==<br>
> exec_list_get_tail(cf_list);<br>
> + switch (cf_node->type) {<br>
> + case nir_cf_node_block:<br>
> + break;<br>
> +<br>
> + case nir_cf_node_if: {<br>
> + nir_if *nif = nir_cf_node_as_if(cf_node);<br>
> + if (lower_trivial_continues_list(<wbr>&nif->then_list,<br>
> at_loop_tail, loop))<br>
> + progress = true;<br>
> + if (lower_trivial_continues_list(<wbr>&nif->else_list,<br>
> at_loop_tail, loop))<br>
> + progress = true;<br>
> + break;<br>
> + }<br>
> +<br>
> + case nir_cf_node_loop: {<br>
> + nir_loop *loop = nir_cf_node_as_loop(cf_node);<br>
> + if (lower_trivial_continues_list(<wbr>&loop->body, true, loop))<br>
> + progress = true;<br>
> + if<br>
> (lower_trivial_continues_block<wbr>(nir_loop_last_block(loop), loop))<br>
> + progress = true;<br>
> + break;<br>
> + }<br>
> +<br>
> + case nir_cf_node_function:<br>
> + unreachable("Invalid cf type");<br>
> + }<br>
> + }<br>
> +<br>
> + return progress;<br>
> +}<br>
> +<br>
> +/**<br>
> + * This simple pass gets rid of any "trivial" continues, i.e. those<br>
> that are<br>
> + * at the tail of a loop where we can just delete the continue<br>
> instruction and<br>
> + * control-flow will naturally and harmlessly take us back to the<br>
> top of the<br>
> + * loop.<br>
> + */<br>
> +bool<br>
> +nir_opt_trivial_continues(nir<wbr>_shader *shader)<br>
> +{<br>
> + bool progress = false;<br>
> +<br>
> + nir_foreach_function(funct<wbr>ion, shader) {<br>
> + if (function->impl == NULL)<br>
> + continue;<br>
> +<br>
> + /* First we run the simple pass to get rid of pesky continues<br>
> */<br>
> + if (lower_trivial_continues_list(<wbr>&function->impl->body, false,<br>
> NULL)) {<br>
> + nir_metadata_preserv<wbr>e(function->impl, nir_metadata_none);<br>
> +<br>
> + /* If that made progress, we're no longer really in SSA<br>
> form. Go<br>
> + * back into SSA form and clean up.<br>
> + */<br>
> + nir_convert_to_ssa_i<wbr>mpl(function->impl);<br>
> + nir_copy_prop_impl(f<wbr>unction->impl);<br>
> + nir_opt_dce_impl(fun<wbr>ction->impl);<br>
<br>
</div>Just curious why you explicitly call these rather than just let them<br>
get cleaned up normally? Does it cut compile times to get rid of them<br>
straight away?<br></blockquote></div></div></div><div dir="auto"><br></div></div></div><div dir="auto">The nir_opt_if pass I wrote relies on this pass. Of this pass is run, it also needs copy_prop and dce. This potentially allows is to drop an iteration of the optimization loop. Another option would be to do this in brw_nir.c:</div><div dir="auto"><br></div><div dir="auto">if (OPT(nir_opt_trivial_continues<wbr>)) {</div><div dir="auto"> OPT(nir_opt_copy_prop);</div><div dir="auto"> OPT(nir_opt_dce);</div><div dir="auto">}</div><div dir="auto"><br></div><div dir="auto">I don't have a strong preference either way.</div></div></blockquote><div><br></div><div>I've updated the brw_nir code to do this for us:<br><br><a href="https://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/nir-opt-continues-v1.1&id=dd7f6934063b27101a4face52fd8bc3c8cadea78" target="_blank">https://cgit.freedesktop.org/~<wbr>jekstrand/mesa/commit/?h=wip/<wbr>nir-opt-continues-v1.1&id=<wbr>dd7f6934063b27101a4face52fd8bc<wbr>3c8cadea78</a><br><br></div><div>I've also deleted 3/6 because it's no longer needed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><span class="m_6473124787206035435gmail-"><div dir="auto"><br></div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="m_6473124787206035435gmail-m_7302714870370531757quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Anyway this looks good to me and very easy to follow:<br>
<br>
Reviewed-by: Timothy Arceri <<a href="mailto:timothy.arceri@collabora.com" target="_blank">timothy.arceri@collabora.com</a>><br>
<div class="m_6473124787206035435gmail-m_7302714870370531757elided-text"><br>
> + progress = true;<br>
> + }<br>
> + }<br>
> +<br>
> + return progress;<br>
> +}</div></blockquote></div></div></div></span></div>
</blockquote></div><br></div></div>