[Mesa-dev] [PATCH v2 2/4] i965/fs: Make get_timestamp() return an fs_inst * rather than emitting.

[Pohjolainen, Topi]

On Fri, Feb 27, 2015 at 11:15:35AM -0800, Kenneth Graunke 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.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 24 +++++++++++++-----------
>  src/mesa/drivers/dri/i965/brw_fs.h   |  2 +-
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> Yikes, good catch!  Thanks for the review, Topi!
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 9c6f084..d65f1f1 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -680,8 +680,8 @@ fs_visitor::type_size(const struct glsl_type *type)
>     return 0;
>  }
>  
> -fs_reg
> -fs_visitor::get_timestamp()
> +fs_inst *
> +fs_visitor::timestamp_read()
>  {
>     assert(brw->gen >= 7);
>  
> @@ -692,12 +692,6 @@ 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));
> -   /* We want to read the 3 fields we care about even if it's not enabled in
> -    * the dispatch.
> -    */
> -   mov->force_writemask_all = true;
> -
>     /* The caller wants the low 32 bits of the timestamp.  Since it's running
>      * at the GPU clock rate of ~1.2ghz, it will roll over every ~3 seconds,
>      * which is plenty of time for our purposes.  It is identical across the
> @@ -710,14 +704,21 @@ fs_visitor::get_timestamp()
>      */
>     dst.set_smear(0);
>  
> -   return dst;
> +   fs_inst *mov = MOV(dst, ts);

Previously the smear wasn't set for the destination in the instruction
itself. I had to check what set_smear() really does. It also sets stride to
zero which the original logic left to the init value of one. I guess this is
not what you intented?

> +   /* We want to read the 3 fields we care about even if it's not enabled in
> +    * the dispatch.
> +    */
> +   mov->force_writemask_all = true;
> +
> +   return mov;
>  }
>  
>  void
>  fs_visitor::emit_shader_time_begin()
>  {
>     current_annotation = "shader time start";
> -   shader_start_time = get_timestamp();
> +   fs_inst *tm_read = emit(timestamp_read());
> +   shader_start_time = tm_read->dst;
>  }
>  
>  void
> @@ -753,7 +754,8 @@ fs_visitor::emit_shader_time_end()
>        unreachable("fs_visitor::emit_shader_time_end missing code");
>     }
>  
> -   fs_reg shader_end_time = get_timestamp();
> +   fs_inst *tm_read = emit(timestamp_read());
> +   fs_reg shader_end_time = tm_read->dst;
>  
>     /* Check that there weren't any timestamp reset events (assuming these
>      * were the only two timestamp reads that happened).
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index be1c8a1..f8044f8 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -399,7 +399,7 @@ public:
>     void resolve_ud_negate(fs_reg *reg);
>     void resolve_bool_comparison(ir_rvalue *rvalue, fs_reg *reg);
>  
> -   fs_reg get_timestamp();
> +   fs_inst *timestamp_read();
>  
>     struct brw_reg interp_reg(int location, int channel);
>     void setup_uniform_values(ir_variable *ir);
> -- 
> 2.2.2
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev