Re: [Mesa-dev] [PATCH 10/21] i965: Only add the wpos state reference if we lowered something

[Jason Ekstrand]

On Wed, Oct 18, 2017 at 6:11 AM, Eero Tamminen 
wrote:

> Hi Jason,
>
> This commit dropped SynMark2 v7 OglShMapPcf performance by 1-2% on GT2
> versions of SKL, KBL and CFL.  It may have improved it by same amount on
> SKL GT4e. Effects on other machines were within normal variation.
>

That's very surprising.  Are you 100% sure it's caused by *this* commit?

--Jason


>
> - Eero
>
>
> On 30.09.2017 00:25, Jason Ekstrand wrote:
>
>> Otherwise, in the ARB program case _mesa_add_state_reference may grow
>> the parameter array which will cause brw_nir_setup_arb_uniforms to write
>> past the end of the param array because it only looks at the parameter
>> list length but the parma array is allocated based on nir->num_uniforms.
>> The only reason this hasn't caused us problems is because we are padding
>> out the param array for fragment programs unnecessarily.
>> ---
>>   src/mesa/drivers/dri/i965/brw_program.c | 9 +
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_program.c
>> b/src/mesa/drivers/dri/i965/brw_program.c
>> index ee464fc..7eec6f7 100644
>> --- a/src/mesa/drivers/dri/i965/brw_program.c
>> +++ b/src/mesa/drivers/dri/i965/brw_program.c
>> @@ -88,8 +88,6 @@ brw_create_nir(struct brw_context *brw,
>>  }
>>  nir_validate_shader(nir);
>>   -   (void)progress;
>> -
>>  nir = brw_preprocess_nir(brw->screen->compiler, nir);
>>if (stage == MESA_SHADER_FRAGMENT) {
>> @@ -98,10 +96,13 @@ brw_create_nir(struct brw_context *brw,
>>.fs_coord_pixel_center_integer = 1,
>>.fs_coord_origin_upper_left = 1,
>> };
>> -  _mesa_add_state_reference(prog->Parameters,
>> -(gl_state_index *)
>> wpos_options.state_tokens);
>>   +  progress = false;
>> NIR_PASS(progress, nir, nir_lower_wpos_ytransform, _options);
>> +  if (progress) {
>> + _mesa_add_state_reference(prog->Parameters,
>> +   (gl_state_index *)
>> wpos_options.state_tokens);
>> +  }
>>  }
>>NIR_PASS(progress, nir, nir_lower_system_values);
>>
>>
> ___
> 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 10/21] i965: Only add the wpos state reference if we lowered something

[Eero Tamminen]

Hi Jason,

This commit dropped SynMark2 v7 OglShMapPcf performance by 1-2% on GT2 
versions of SKL, KBL and CFL.  It may have improved it by same amount on 
SKL GT4e. Effects on other machines were within normal variation.



- Eero

On 30.09.2017 00:25, Jason Ekstrand wrote:

Otherwise, in the ARB program case _mesa_add_state_reference may grow
the parameter array which will cause brw_nir_setup_arb_uniforms to write
past the end of the param array because it only looks at the parameter
list length but the parma array is allocated based on nir->num_uniforms.
The only reason this hasn't caused us problems is because we are padding
out the param array for fragment programs unnecessarily.
---
  src/mesa/drivers/dri/i965/brw_program.c | 9 +
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_program.c 
b/src/mesa/drivers/dri/i965/brw_program.c
index ee464fc..7eec6f7 100644
--- a/src/mesa/drivers/dri/i965/brw_program.c
+++ b/src/mesa/drivers/dri/i965/brw_program.c
@@ -88,8 +88,6 @@ brw_create_nir(struct brw_context *brw,
 }
 nir_validate_shader(nir);
  
-   (void)progress;

-
 nir = brw_preprocess_nir(brw->screen->compiler, nir);
  
 if (stage == MESA_SHADER_FRAGMENT) {

@@ -98,10 +96,13 @@ brw_create_nir(struct brw_context *brw,
   .fs_coord_pixel_center_integer = 1,
   .fs_coord_origin_upper_left = 1,
};
-  _mesa_add_state_reference(prog->Parameters,
-(gl_state_index *) wpos_options.state_tokens);
  
+  progress = false;

NIR_PASS(progress, nir, nir_lower_wpos_ytransform, _options);
+  if (progress) {
+ _mesa_add_state_reference(prog->Parameters,
+   (gl_state_index *) 
wpos_options.state_tokens);
+  }
 }
  
 NIR_PASS(progress, nir, nir_lower_system_values);




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

Re: [Mesa-dev] [PATCH 10/21] i965: Only add the wpos state reference if we lowered something

[Jason Ekstrand]

On Wed, Oct 4, 2017 at 7:31 PM, Jordan Justen 
wrote:

> On 2017-09-29 14:25:10, Jason Ekstrand wrote:
> > Otherwise, in the ARB program case _mesa_add_state_reference may grow
> > the parameter array which will cause brw_nir_setup_arb_uniforms to write
> > past the end of the param array because it only looks at the parameter
> > list length but the parma array is allocated based on nir->num_uniforms.
> > The only reason this hasn't caused us problems is because we are padding
> > out the param array for fragment programs unnecessarily.
> > ---
> >  src/mesa/drivers/dri/i965/brw_program.c | 9 +
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_program.c
> b/src/mesa/drivers/dri/i965/brw_program.c
> > index ee464fc..7eec6f7 100644
> > --- a/src/mesa/drivers/dri/i965/brw_program.c
> > +++ b/src/mesa/drivers/dri/i965/brw_program.c
> > @@ -88,8 +88,6 @@ brw_create_nir(struct brw_context *brw,
> > }
> > nir_validate_shader(nir);
> >
> > -   (void)progress;
> > -
> > nir = brw_preprocess_nir(brw->screen->compiler, nir);
> >
> > if (stage == MESA_SHADER_FRAGMENT) {
> > @@ -98,10 +96,13 @@ brw_create_nir(struct brw_context *brw,
> >   .fs_coord_pixel_center_integer = 1,
> >   .fs_coord_origin_upper_left = 1,
> >};
> > -  _mesa_add_state_reference(prog->Parameters,
> > -(gl_state_index *)
> wpos_options.state_tokens);
> >
> > +  progress = false;
>
> Should we move the `progress` declaration here?
>
> >NIR_PASS(progress, nir, nir_lower_wpos_ytransform, _options);
> > +  if (progress) {
> > + _mesa_add_state_reference(prog->Parameters,
> > +   (gl_state_index *)
> wpos_options.state_tokens);
> > +  }
> > }
> >
> > NIR_PASS(progress, nir, nir_lower_system_values);
>
> And convert this to NIR_PASS_V?
>

Sure.  I thought about that but didn't.  Done.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/21] i965: Only add the wpos state reference if we lowered something

[Jordan Justen]

On 2017-09-29 14:25:10, Jason Ekstrand wrote:
> Otherwise, in the ARB program case _mesa_add_state_reference may grow
> the parameter array which will cause brw_nir_setup_arb_uniforms to write
> past the end of the param array because it only looks at the parameter
> list length but the parma array is allocated based on nir->num_uniforms.
> The only reason this hasn't caused us problems is because we are padding
> out the param array for fragment programs unnecessarily.
> ---
>  src/mesa/drivers/dri/i965/brw_program.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_program.c 
> b/src/mesa/drivers/dri/i965/brw_program.c
> index ee464fc..7eec6f7 100644
> --- a/src/mesa/drivers/dri/i965/brw_program.c
> +++ b/src/mesa/drivers/dri/i965/brw_program.c
> @@ -88,8 +88,6 @@ brw_create_nir(struct brw_context *brw,
> }
> nir_validate_shader(nir);
>  
> -   (void)progress;
> -
> nir = brw_preprocess_nir(brw->screen->compiler, nir);
>  
> if (stage == MESA_SHADER_FRAGMENT) {
> @@ -98,10 +96,13 @@ brw_create_nir(struct brw_context *brw,
>   .fs_coord_pixel_center_integer = 1,
>   .fs_coord_origin_upper_left = 1,
>};
> -  _mesa_add_state_reference(prog->Parameters,
> -(gl_state_index *) 
> wpos_options.state_tokens);
>  
> +  progress = false;

Should we move the `progress` declaration here?

>NIR_PASS(progress, nir, nir_lower_wpos_ytransform, _options);
> +  if (progress) {
> + _mesa_add_state_reference(prog->Parameters,
> +   (gl_state_index *) 
> wpos_options.state_tokens);
> +  }
> }
>  
> NIR_PASS(progress, nir, nir_lower_system_values);

And convert this to NIR_PASS_V?

-Jordan

> -- 
> 2.5.0.400.gff86faf
> 
> ___
> 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] [PATCH 10/21] i965: Only add the wpos state reference if we lowered something

[Jason Ekstrand]

Otherwise, in the ARB program case _mesa_add_state_reference may grow
the parameter array which will cause brw_nir_setup_arb_uniforms to write
past the end of the param array because it only looks at the parameter
list length but the parma array is allocated based on nir->num_uniforms.
The only reason this hasn't caused us problems is because we are padding
out the param array for fragment programs unnecessarily.
---
 src/mesa/drivers/dri/i965/brw_program.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_program.c 
b/src/mesa/drivers/dri/i965/brw_program.c
index ee464fc..7eec6f7 100644
--- a/src/mesa/drivers/dri/i965/brw_program.c
+++ b/src/mesa/drivers/dri/i965/brw_program.c
@@ -88,8 +88,6 @@ brw_create_nir(struct brw_context *brw,
}
nir_validate_shader(nir);
 
-   (void)progress;
-
nir = brw_preprocess_nir(brw->screen->compiler, nir);
 
if (stage == MESA_SHADER_FRAGMENT) {
@@ -98,10 +96,13 @@ brw_create_nir(struct brw_context *brw,
  .fs_coord_pixel_center_integer = 1,
  .fs_coord_origin_upper_left = 1,
   };
-  _mesa_add_state_reference(prog->Parameters,
-(gl_state_index *) wpos_options.state_tokens);
 
+  progress = false;
   NIR_PASS(progress, nir, nir_lower_wpos_ytransform, _options);
+  if (progress) {
+ _mesa_add_state_reference(prog->Parameters,
+   (gl_state_index *) 
wpos_options.state_tokens);
+  }
}
 
NIR_PASS(progress, nir, nir_lower_system_values);
-- 
2.5.0.400.gff86faf

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