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

[Jason Ekstrand]

On Thu, Jan 12, 2017 at 12:38 PM, Matt Turner <mattst88 at gmail.com> wrote:

> 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.
>

Thanks.  I'll fix that up.


> >      */
> >     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>
>

Thanks!  I'd almost forgotten about this patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170112/8d027e80/attachment.html>