[Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks

[Lionel Landwerlin]

Just some style comments, feel free to ignore them.

On 20/07/17 12:35, aravindan.muthukumar at intel.com wrote:
> From: Aravindan Muthukumar <aravindan.muthukumar at intel.com>
>
> This patch improves CPI Rate(Cycles per Instruction)
> and branch mispredict for i965. The function check_state()
> was showing CPI retired rate.
>
> Performance stats with android:
> CPI retired lowered by 28% (lower is better)
> Branch missprediction lowered by 13% (lower is better)
> 3DMark improved by 2%
>
> The dissassembly doesn't show difference, although above
> results were observed with patch.
>
> Signed-off-by: Aravindan Muthukumar <aravindan.muthukumar at intel.com>
> Signedd-off-by: Yogesh Marathe <yogesh.marathe at intel.com>
> Tested-by: Asish <asish at intel.com>
> ---
>
> Changes since V1:
> - Removed memset() change
> - Changed commit message as per review comments
>
>   src/mesa/drivers/dri/i965/brw_defines.h      |  4 ++++
>   src/mesa/drivers/dri/i965/brw_state_upload.c | 12 ++++++++----
>   2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index 2a8dbf8..8c9a510 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -1687,3 +1687,7 @@ enum brw_pixel_shader_coverage_mask_mode {
>   # define CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE (1 << 4)
>   
>   #endif
> +
> +/* Checking the state of mesa and brw before emitting atoms */
> +#define CHECK_BRW_STATE(a,b) ((a.mesa & b.mesa) | (a.brw & b.brw))
> +
> diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c b/src/mesa/drivers/dri/i965/brw_state_upload.c
> index acaa97e..1c8b969 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> @@ -443,10 +443,8 @@ check_and_emit_atom(struct brw_context *brw,
>                       struct brw_state_flags *state,
>                       const struct brw_tracked_state *atom)
>   {
> -   if (check_state(state, &atom->dirty)) {
>         atom->emit(brw);
>         merge_ctx_state(brw, state);
> -   }

You might want to re-indent this.
Also maybe that function can be rename since it won't check anything 
anymore.

>   }
>   
>   static inline void
> @@ -541,7 +539,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
>   	 const struct brw_tracked_state *atom = &atoms[i];
>   	 struct brw_state_flags generated;
>   
> -         check_and_emit_atom(brw, &state, atom);
> +         /* Checking the state and emitting atoms */
> +         if (CHECK_BRW_STATE(state, atom->dirty)) {
> +            check_and_emit_atom(brw, &state, atom);
> +         }
>   
>   	 accumulate_state(&examined, &atom->dirty);
>   
> @@ -558,7 +559,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
>         for (i = 0; i < num_atoms; i++) {
>   	 const struct brw_tracked_state *atom = &atoms[i];
>   
> -         check_and_emit_atom(brw, &state, atom);
> +         /* Checking the state and emitting atoms */
> +         if (CHECK_BRW_STATE(state, atom->dirty)) {
> +            check_and_emit_atom(brw, &state, atom);
> +         }
>         }
>      }
>   


Why not replacing the last call to check_state() by CHECK_BRW_STATE() 
and get rid of that function altogether?