<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jan 12, 2017 at 12:38 PM, Matt Turner <span dir="ltr"><<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Dec 1, 2016 at 1:51 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
> This fixes a bug in code motion that occurred when the best block is the<br>
> same as the schedule early block.  In this case, because we're checking<br>
> (lca != def->parent_instr->block) at the top of the loop, we never get to<br>
> the check for loop depth so we wouldn't move it out of the loop.  This<br>
> commit reworks the loop to be a simple for loop up the dominator chain and<br>
> we place the (lca != def->parent_instr->block) check at the end of the<br>
> loop.<br>
> ---<br>
>  src/compiler/nir/nir_opt_gcm.c | 11 ++++++-----<br>
>  1 file changed, 6 insertions(+), 5 deletions(-)<br>
><br>
> diff --git a/src/compiler/nir/nir_opt_<wbr>gcm.c b/src/compiler/nir/nir_opt_<wbr>gcm.c<br>
> index 77eb8e6..9d7f59c 100644<br>
> --- a/src/compiler/nir/nir_opt_<wbr>gcm.c<br>
> +++ b/src/compiler/nir/nir_opt_<wbr>gcm.c<br>
> @@ -326,12 +326,13 @@ gcm_schedule_late_def(nir_ssa_<wbr>def *def, void *void_state)<br>
>      * as far outside loops as we can get.<br>
<br>
</span>In this comment, the first line says "know" instead of "now". Please<br>
fix that while we're here.<span class=""><br></span></blockquote><div><br></div><div>Thanks.  I'll fix that up.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>      */<br>
>     nir_block *best = lca;<br>
> -   while (lca != def->parent_instr->block) {<br>
> -      assert(lca);<br>
> -      if (state->blocks[lca->index].<wbr>loop_depth <<br>
> +   for (nir_block *block = lca; block != NULL; block = block->imm_dom) {<br>
> +      if (state->blocks[block->index].<wbr>loop_depth <<br>
>            state->blocks[best->index].<wbr>loop_depth)<br>
> -         best = lca;<br>
> -      lca = lca->imm_dom;<br>
> +         best = block;<br>
> +<br>
> +      if (block == def->parent_instr->block)<br>
> +         break;<br>
>     }<br>
<br>
</span>I had to come up with an example and walk through it to understand the<br>
issue, but I now understand and agree that this is the correct fix.<br>
<br>
This patch is<br>
<br>
Reviewed-by: Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>><br>
</blockquote></div><br></div><div class="gmail_extra">Thanks!  I'd almost forgotten about this patch.<br></div></div>