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

[Marathe, Yogesh]

> Just some style comments, feel free to ignore them.
> 

Both comments are relevant, will address them in V3. Thanks Lionel.

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