Re: [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  bora.com> wrote:
> > On Wed, 2016-12-21 at 21:11 -0800, Jason Ekstrand wrote:
> > > On Dec 21, 2016 9:17 PM, "Timothy Arceri"  > 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 
> > >
> > > >    if (nir->options->max_unroll_iterations != 0) {
> > > >   OPT(nir_opt_loop_unroll, indirect_mask);
> > > >    }
> > >
> > > ___
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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.arc...@collabora.com> wrote:

> On Wed, 2016-12-21 at 21:11 -0800, Jason Ekstrand wrote:
> > On Dec 21, 2016 9:17 PM, "Timothy Arceri"  > 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 
> >
> > >if (nir->options->max_unroll_iterations != 0) {
> > >   OPT(nir_opt_loop_unroll, indirect_mask);
> > >}
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Timothy Arceri]

On Wed, 2016-12-21 at 21:11 -0800, Jason Ekstrand wrote:
> On Dec 21, 2016 9:17 PM, "Timothy Arceri"  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. 

> 
> Reviewed-by: Timothy Arceri 
> 
> >    if (nir->options->max_unroll_iterations != 0) {
> >   OPT(nir_opt_loop_unroll, indirect_mask);
> >    }
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

On Dec 21, 2016 9:17 PM, "Timothy Arceri" 
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?

Reviewed-by: Timothy Arceri 

>if (nir->options->max_unroll_iterations != 0) {
>   OPT(nir_opt_loop_unroll, indirect_mask);
>}
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Timothy Arceri]

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:

Reviewed-by: Timothy Arceri 

>    if (nir->options->max_unroll_iterations != 0) {
>   OPT(nir_opt_loop_unroll, indirect_mask);
>    }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

---
 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);
   if (nir->options->max_unroll_iterations != 0) {
  OPT(nir_opt_loop_unroll, indirect_mask);
   }
-- 
2.5.0.400.gff86faf

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev