intel: compiler: remove dead local variables at optimization pass

Submitted by Lionel Landwerlin on Sept. 3, 2018, 3:47 p.m.

Details

Message ID 20180903154703.24741-1-lionel.g.landwerlin@intel.com
State New
Headers show
Series "intel: compiler: remove dead local variables at optimization pass" ( rev: 2 ) in Mesa

Browsing this patch as part of:
"intel: compiler: remove dead local variables at optimization pass" rev 2 in Mesa
<< prev patch [1/1] next patch >>

Commit Message

Lionel Landwerlin Sept. 3, 2018, 3:47 p.m.
We're hitting an assert in gfxbench because one of the local variable
is a sampler (according to Jason this isn't valid) :

testfw_app: ../src/compiler/nir_types.cpp:551: void glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned int*): Assertion `!"type does not have a natural size"' failed.

Since this particular variable isn't used and it can be eliminated by
removing unused local variables in the optimization pass. This makes
sense also for valid local variables.

v2: Move additional local variable removal out of optimization loop,
    but before large constant removal (Jason/Lionel)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 src/intel/compiler/brw_nir.c | 5 +++++
 1 file changed, 5 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index ef5034d1e1e..8c1bcb99f8c 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -673,6 +673,11 @@  brw_preprocess_nir(const struct brw_compiler *compiler, nir_shader *nir)
 
    nir = brw_nir_optimize(nir, compiler, is_scalar, true);
 
+   /* Workaround Gfxbench unused local sampler variable which will trigger an
+    * assert in the opt_large_constants pass.
+    */
+   OPT(nir_remove_dead_variables, nir_var_local);
+
    /* This needs to be run after the first optimization pass but before we
     * lower indirect derefs away
     */

Comments

On 03/09/2018 16:47, Lionel Landwerlin wrote:
> We're hitting an assert in gfxbench because one of the local variable
> is a sampler (according to Jason this isn't valid) :
>
> testfw_app: ../src/compiler/nir_types.cpp:551: void glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned int*): Assertion `!"type does not have a natural size"' failed.
>
> Since this particular variable isn't used and it can be eliminated by
> removing unused local variables in the optimization pass. This makes
> sense also for valid local variables.
>
> v2: Move additional local variable removal out of optimization loop,
>      but before large constant removal (Jason/Lionel)
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>


Forgot to add Eero's bug : 
https://bugs.freedesktop.org/show_bug.cgi?id=107806


> ---
>   src/intel/compiler/brw_nir.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> index ef5034d1e1e..8c1bcb99f8c 100644
> --- a/src/intel/compiler/brw_nir.c
> +++ b/src/intel/compiler/brw_nir.c
> @@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler *compiler, nir_shader *nir)
>   
>      nir = brw_nir_optimize(nir, compiler, is_scalar, true);
>   
> +   /* Workaround Gfxbench unused local sampler variable which will trigger an
> +    * assert in the opt_large_constants pass.
> +    */
> +   OPT(nir_remove_dead_variables, nir_var_local);
> +
>      /* This needs to be run after the first optimization pass but before we
>       * lower indirect derefs away
>       */
On September 3, 2018 10:47:11 Lionel Landwerlin 
<lionel.g.landwerlin@intel.com> wrote:

> We're hitting an assert in gfxbench because one of the local variable
> is a sampler (according to Jason this isn't valid) :
>
> testfw_app: ../src/compiler/nir_types.cpp:551: void 
> glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned 
> int*): Assertion `!"type does not have a natural size"' failed.
>
> Since this particular variable isn't used and it can be eliminated by
> removing unused local variables in the optimization pass. This makes
> sense also for valid local variables.
>
> v2: Move additional local variable removal out of optimization loop,
>    but before large constant removal (Jason/Lionel)

Oh, I meant to make it the last thing brw_optimize_nir does because the 
other call to this pass is also right after brw_optimize_nir.  This is also 
fine.

Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
> src/intel/compiler/brw_nir.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> index ef5034d1e1e..8c1bcb99f8c 100644
> --- a/src/intel/compiler/brw_nir.c
> +++ b/src/intel/compiler/brw_nir.c
> @@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler 
> *compiler, nir_shader *nir)
>
>    nir = brw_nir_optimize(nir, compiler, is_scalar, true);
>
> +   /* Workaround Gfxbench unused local sampler variable which will trigger an
> +    * assert in the opt_large_constants pass.
> +    */
> +   OPT(nir_remove_dead_variables, nir_var_local);
> +
>    /* This needs to be run after the first optimization pass but before we
>     * lower indirect derefs away
>     */
> --
> 2.19.0.rc1
On 03/09/2018 17:15, Jason Ekstrand wrote:
> On September 3, 2018 10:47:11 Lionel Landwerlin 
> <lionel.g.landwerlin@intel.com> wrote:
>
>> We're hitting an assert in gfxbench because one of the local variable
>> is a sampler (according to Jason this isn't valid) :
>>
>> testfw_app: ../src/compiler/nir_types.cpp:551: void 
>> glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, 
>> unsigned int*): Assertion `!"type does not have a natural size"' failed.
>>
>> Since this particular variable isn't used and it can be eliminated by
>> removing unused local variables in the optimization pass. This makes
>> sense also for valid local variables.
>>
>> v2: Move additional local variable removal out of optimization loop,
>>    but before large constant removal (Jason/Lionel)
>
> Oh, I meant to make it the last thing brw_optimize_nir does because 
> the other call to this pass is also right after brw_optimize_nir.  
> This is also fine.
>
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>


Oh right, I'll update.


>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>> src/intel/compiler/brw_nir.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
>> index ef5034d1e1e..8c1bcb99f8c 100644
>> --- a/src/intel/compiler/brw_nir.c
>> +++ b/src/intel/compiler/brw_nir.c
>> @@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler 
>> *compiler, nir_shader *nir)
>>
>>    nir = brw_nir_optimize(nir, compiler, is_scalar, true);
>>
>> +   /* Workaround Gfxbench unused local sampler variable which will 
>> trigger an
>> +    * assert in the opt_large_constants pass.
>> +    */
>> +   OPT(nir_remove_dead_variables, nir_var_local);
>> +
>>    /* This needs to be run after the first optimization pass but 
>> before we
>>     * lower indirect derefs away
>>     */
>> -- 
>> 2.19.0.rc1
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Lionel, should we also include this in the stable queues ?


On Mon, 2018-09-03 at 16:47 +0100, Lionel Landwerlin wrote:
> We're hitting an assert in gfxbench because one of the local variable
> is a sampler (according to Jason this isn't valid) :
> 
> testfw_app: ../src/compiler/nir_types.cpp:551: void glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned int*): Assertion `!"type does not have a natural size"' failed.
> 
> Since this particular variable isn't used and it can be eliminated by
> removing unused local variables in the optimization pass. This makes
> sense also for valid local variables.
> 
> v2: Move additional local variable removal out of optimization loop,
>     but before large constant removal (Jason/Lionel)
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  src/intel/compiler/brw_nir.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> index ef5034d1e1e..8c1bcb99f8c 100644
> --- a/src/intel/compiler/brw_nir.c
> +++ b/src/intel/compiler/brw_nir.c
> @@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler *compiler, nir_shader *nir)
>  
>     nir = brw_nir_optimize(nir, compiler, is_scalar, true);
>  
> +   /* Workaround Gfxbench unused local sampler variable which will trigger an
> +    * assert in the opt_large_constants pass.
> +    */
> +   OPT(nir_remove_dead_variables, nir_var_local);
> +
>     /* This needs to be run after the first optimization pass but before we
>      * lower indirect derefs away
>      */
Doesn't matter too much

On Tue, Sep 4, 2018 at 3:45 PM Andres Gomez <agomez@igalia.com> wrote:

> Lionel, should we also include this in the stable queues ?
>
>
> On Mon, 2018-09-03 at 16:47 +0100, Lionel Landwerlin wrote:
> > We're hitting an assert in gfxbench because one of the local variable
> > is a sampler (according to Jason this isn't valid) :
> >
> > testfw_app: ../src/compiler/nir_types.cpp:551: void
> glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned
> int*): Assertion `!"type does not have a natural size"' failed.
> >
> > Since this particular variable isn't used and it can be eliminated by
> > removing unused local variables in the optimization pass. This makes
> > sense also for valid local variables.
> >
> > v2: Move additional local variable removal out of optimization loop,
> >     but before large constant removal (Jason/Lionel)
> >
> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > ---
> >  src/intel/compiler/brw_nir.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> > index ef5034d1e1e..8c1bcb99f8c 100644
> > --- a/src/intel/compiler/brw_nir.c
> > +++ b/src/intel/compiler/brw_nir.c
> > @@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler
> *compiler, nir_shader *nir)
> >
> >     nir = brw_nir_optimize(nir, compiler, is_scalar, true);
> >
> > +   /* Workaround Gfxbench unused local sampler variable which will
> trigger an
> > +    * assert in the opt_large_constants pass.
> > +    */
> > +   OPT(nir_remove_dead_variables, nir_var_local);
> > +
> >     /* This needs to be run after the first optimization pass but before
> we
> >      * lower indirect derefs away
> >      */
> --
> Br,
>
> Andres
>
Thanks for the prompt reply, Jason. I will skip this patch, then.


On Tue, 2018-09-04 at 15:51 -0500, Jason Ekstrand wrote:
> Doesn't matter too much
> 
> On Tue, Sep 4, 2018 at 3:45 PM Andres Gomez <agomez@igalia.com> wrote:
> > Lionel, should we also include this in the stable queues ?
> > 
> > 
> > On Mon, 2018-09-03 at 16:47 +0100, Lionel Landwerlin wrote:
> > > We're hitting an assert in gfxbench because one of the local variable
> > > is a sampler (according to Jason this isn't valid) :
> > > 
> > > testfw_app: ../src/compiler/nir_types.cpp:551: void glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned int*): Assertion `!"type does not have a natural size"' failed.
> > > 
> > > Since this particular variable isn't used and it can be eliminated by
> > > removing unused local variables in the optimization pass. This makes
> > > sense also for valid local variables.
> > > 
> > > v2: Move additional local variable removal out of optimization loop,
> > >     but before large constant removal (Jason/Lionel)
> > > 
> > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > > ---
> > >  src/intel/compiler/brw_nir.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> > > index ef5034d1e1e..8c1bcb99f8c 100644
> > > --- a/src/intel/compiler/brw_nir.c
> > > +++ b/src/intel/compiler/brw_nir.c
> > > @@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler *compiler, nir_shader *nir)
> > >  
> > >     nir = brw_nir_optimize(nir, compiler, is_scalar, true);
> > >  
> > > +   /* Workaround Gfxbench unused local sampler variable which will trigger an
> > > +    * assert in the opt_large_constants pass.
> > > +    */
> > > +   OPT(nir_remove_dead_variables, nir_var_local);
> > > +
> > >     /* This needs to be run after the first optimization pass but before we
> > >      * lower indirect derefs away
> > >      */