[Mesa-dev] [PATCH 4/5] i965/fs: Make get_timestamp() pass back the MOV rather than emitting it.

[Pohjolainen, Topi]

On Wed, Mar 11, 2015 at 10:17:54AM +0200, Pohjolainen, Topi wrote:
> On Mon, Mar 09, 2015 at 11:40:19AM -0700, Matt Turner wrote:
> > On Sun, Mar 8, 2015 at 1:08 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > > This makes another part of the INTEL_DEBUG=shader_time code emittable
> > > at arbitrary locations, rather than just at the end of the instruction
> > > stream.
> > >
> > > v2: Don't lose smear!  Caught by Topi Pohjolainen.
> > > v3: Don't set smear on the destination of the MOV.  Thanks Topi!
> > >
> > > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_fs.cpp | 19 +++++++++++++++----
> > >  src/mesa/drivers/dri/i965/brw_fs.h   |  2 +-
> > >  2 files changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > > index 682841b..8f11600 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > > @@ -677,8 +677,14 @@ fs_visitor::type_size(const struct glsl_type *type)
> > >     return 0;
> > >  }
> > >
> > > +/**
> > > + * Create a MOV to read the timestamp register.
> > > + *
> > > + * The caller is responsible for emitting the MOV.  The return value is
> > > + * the destination of the MOV, with extra parameters set.
> > > + */
> > >  fs_reg
> > > -fs_visitor::get_timestamp()
> > > +fs_visitor::get_timestamp(fs_inst **out_mov)
> > >  {
> > >     assert(brw->gen >= 7);
> > >
> > > @@ -689,7 +695,7 @@ fs_visitor::get_timestamp()
> > >
> > >     fs_reg dst = fs_reg(GRF, alloc.allocate(1), BRW_REGISTER_TYPE_UD, 4);
> > >
> > > -   fs_inst *mov = emit(MOV(dst, ts));
> > > +   fs_inst *mov = MOV(dst, ts);
> > >     /* We want to read the 3 fields we care about even if it's not enabled in
> > >      * the dispatch.
> > >      */
> > > @@ -707,6 +713,7 @@ fs_visitor::get_timestamp()
> > >      */
> > >     dst.set_smear(0);
> > >
> > > +   *out_mov = mov;
> > >     return dst;
> > >  }
> > >
> > > @@ -714,7 +721,9 @@ void
> > >  fs_visitor::emit_shader_time_begin()
> > >  {
> > >     current_annotation = "shader time start";
> > > -   shader_start_time = get_timestamp();
> > > +   fs_inst *mov;
> > > +   shader_start_time = get_timestamp(&mov);
> > > +   emit(mov);
> > 
> > What do you think about returning the mov instruction from
> > get_timestamp, and then just doing shader_start_time = mov->dst?
> 
> Then each caller would need to additionally set the smear:
> 
> shader_start_time.set_smear(0);

I think I prefer get_timestamp() setting the smear so that the callers
won't miss it. Unless Matt comes up with something, patches 4 and 5 are:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>