[Mesa-dev] [PATCH] nir/gcm: Rework the schedule late loop

[Matt Turner]

On Thu, Dec 1, 2016 at 1:51 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> This fixes a bug in code motion that occurred when the best block is the
> same as the schedule early block.  In this case, because we're checking
> (lca != def->parent_instr->block) at the top of the loop, we never get to
> the check for loop depth so we wouldn't move it out of the loop.  This
> commit reworks the loop to be a simple for loop up the dominator chain and
> we place the (lca != def->parent_instr->block) check at the end of the
> loop.
> ---
>  src/compiler/nir/nir_opt_gcm.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opt_gcm.c b/src/compiler/nir/nir_opt_gcm.c
> index 77eb8e6..9d7f59c 100644
> --- a/src/compiler/nir/nir_opt_gcm.c
> +++ b/src/compiler/nir/nir_opt_gcm.c
> @@ -326,12 +326,13 @@ gcm_schedule_late_def(nir_ssa_def *def, void *void_state)
>      * as far outside loops as we can get.

In this comment, the first line says "know" instead of "now". Please
fix that while we're here.

>      */
>     nir_block *best = lca;
> -   while (lca != def->parent_instr->block) {
> -      assert(lca);
> -      if (state->blocks[lca->index].loop_depth <
> +   for (nir_block *block = lca; block != NULL; block = block->imm_dom) {
> +      if (state->blocks[block->index].loop_depth <
>            state->blocks[best->index].loop_depth)
> -         best = lca;
> -      lca = lca->imm_dom;
> +         best = block;
> +
> +      if (block == def->parent_instr->block)
> +         break;
>     }

I had to come up with an example and walk through it to understand the
issue, but I now understand and agree that this is the correct fix.

This patch is

Reviewed-by: Matt Turner <mattst88 at gmail.com>