[Mesa-dev] [PATCH] i965/vec4: fix record clearing in copy propagation

[Ian Romanick]

On 04/06/2014 09:31 PM, Chia-I Wu wrote:
> From: Chia-I Wu <olv at lunarg.com>
> 
> Given
> 
>   mov vgrf7, vgrf9.xyxz
>   add vgrf9.xyz, vgrf4.xyzw, vgrf5.xyzw
>   add vgrf10.x, vgrf6.xyzw, vgrf7.wwww
> 
> the last instruction would be wrongly changed to
> 
>   add vgrf10.x, vgrf6.xyzw, vgrf9.zzzz
> 
> during copy propagation.
> 
> The issue is that when deciding if a record should be cleared, the old code
> checked for
> 
>   inst->dst.writemask & (1 << ch)
> 
> instead of
> 
>   inst->dst.writemask & (1 << BRW_GET_SWZ(src->swizzle, ch))
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76749
> Signed-off-by: Chia-I Wu <olv at lunarg.com>
> Cc: Jordan Justen <jljusten at gmail.com>
> Cc: Matt Turner <mattst88 at gmail.com>

Mark for stable?  It looks like the same code exists in the 10.1 branch.

The fix looks correct to me, but you should wait for Matt, Eric, or Ken
to comment as well.

Reviewed-by: Ian Romainck <ian.d.romanick at intel.com>

> ---
>  .../drivers/dri/i965/brw_vec4_copy_propagation.cpp  | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> index 3d68f0e..83cf191 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -58,6 +58,21 @@ is_dominated_by_previous_instruction(vec4_instruction *inst)
>  }
>  
>  static bool
> +is_channel_updated(vec4_instruction *inst, src_reg *values[4], int ch)
> +{
> +   const src_reg *src = values[ch];
> +
> +   /* consider GRF only */
> +   assert(inst->dst.file == GRF);
> +   if (!src || src->file != GRF)
> +      return false;
> +
> +   return (src->reg == inst->dst.reg &&
> +	   src->reg_offset == inst->dst.reg_offset &&
> +	   inst->dst.writemask & (1 << BRW_GET_SWZ(src->swizzle, ch)));
> +}
> +
> +static bool
>  try_constant_propagation(vec4_instruction *inst, int arg, src_reg *values[4])
>  {
>     /* For constant propagation, we only handle the same constant
> @@ -357,11 +372,7 @@ vec4_visitor::opt_copy_propagation()
>  	 else {
>  	    for (int i = 0; i < virtual_grf_reg_count; i++) {
>  	       for (int j = 0; j < 4; j++) {
> -		  if (inst->dst.writemask & (1 << j) &&
> -		      cur_value[i][j] &&
> -		      cur_value[i][j]->file == GRF &&
> -		      cur_value[i][j]->reg == inst->dst.reg &&
> -		      cur_value[i][j]->reg_offset == inst->dst.reg_offset) {
> +		  if (is_channel_updated(inst, cur_value[i], j)){
>  		     cur_value[i][j] = NULL;
>  		  }
>  	       }