[Mesa-dev] [PATCH 6/6] i965: Use nir_opt_trivial_continues and nir_opt_if

[Jason Ekstrand]

On Thu, Dec 22, 2016 at 3:57 AM, Timothy Arceri <
timothy.arceri at collabora.com> wrote:

> On Wed, 2016-12-21 at 21:11 -0800, Jason Ekstrand wrote:
> > On Dec 21, 2016 9:17 PM, "Timothy Arceri" <timothy.arceri at collabora.c
> > om> wrote:
> > On Mon, 2016-12-19 at 20:11 -0800, Jason Ekstrand wrote:
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_nir.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_nir.c
> > > b/src/mesa/drivers/dri/i965/brw_nir.c
> > > index 0c1fb44..a091861 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_nir.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_nir.c
> > > @@ -429,6 +429,8 @@ nir_optimize(nir_shader *nir, const struct
> > > brw_compiler *compiler,
> > >        OPT(nir_opt_algebraic);
> > >        OPT(nir_opt_constant_folding);
> > >        OPT(nir_opt_dead_cf);
> > > +      OPT(nir_opt_trivial_continues);
> > > +      OPT(nir_opt_if);
> >
> > I'm seeing regressions in my series for Vulkan (the Vulkan CTS was
> > only
> > enabled for my branch today).  I wonder if we should be applying your
> > series before mine? Either way this or a similar patch is:
> >
> > If you're seeing regressions, there's probably a real bug there.  I
> > assume you applied the else case fix?
>
> Yes its applied. I haven't checked them all but it seems at least some
> are falling over because of a redundant continue at the end of the
> loop. So to me it seems we should probably at least land
> nir_opt_trivial_continues before my series. Loop analysis will bail out
> if we detect a continue in an if branch but we expect no jumps in the
> loop body itself.
>

I did a little digging and came to that same conclusion.  I've got a patch
that fixes it (and, IMHO, cleans a few things up) on my jenkins_vulkan
branch.  We'll see how the CTS likes it but it looks good when I run
dEQP-VK.glsl.loops.* on my laptop.

While I was at it, I realized that we don't yet handle the fairly stupid
case of

while (true) {
   // Do Stuff
   break;
}

I'm not sure if it's our job or the job of dead_cf to handle that, but it
should get handled somewhere.  That smells like a follow-on patch to me.

--Jason


> >
> > Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
> >
> > >        if (nir->options->max_unroll_iterations != 0) {
> > >           OPT(nir_opt_loop_unroll, indirect_mask);
> > >        }
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161222/54a6e318/attachment.html>