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

[Timothy Arceri]

On Thu, 2016-12-22 at 11:42 -0800, Jason Ekstrand wrote:
> On Thu, Dec 22, 2016 at 3:57 AM, Timothy Arceri <timothy.arceri at colla
> bora.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 collabo
> > ra.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.

I was sure we handled by deaf cf this but looking at again it doesn't
look like we do. Should be easy enough to take care of somewhere.

> 
> --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
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev