[Mesa-dev] [PATCH 05/23] i965/fs: fix copy-propagation with suboffset from constants

[Jordan Justen]

On 2016-05-03 05:21:54, Samuel Iglesias Gonsálvez wrote:
> From: Iago Toral Quiroga <itoral at igalia.com>
> 
> The current code ignores the suboffet in the instruction's source
> and just uses the one from the constant. This is not correct
> when the instruction's source is accessing the constant with a
> different type and using the suboffset to select a specific
> chunk of the constant. We generate this kind of code in fp64
> when we want to select only the high 32-bit of a particular
> double constant.
> 
> Instead, we should add any existing suboffset in the
> instruction's source (modulo the size of the entry's type)
> to the suboffset in the constant so we can preserve the orinal
> semantics.
> 
> Prevents that we turn this:
> 
> mov(8) vgrf5:DF, u2<0>:DF
> mov(8) vgrf7:UD, vgrf5+0.4<2>:UD
> 
> Into:
> 
> mov(8) vgrf7:UD, u2<0>:UD
> 
> And instead, with this patch, we produce:
> 
> mov(8) vgrf7:UD, u2+0.4<0>:UD
> ---
>  .../drivers/dri/i965/brw_fs_copy_propagation.cpp   | 23 ++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> index aa4c9c9..5fae10f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -445,8 +445,27 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>     case BAD_FILE:
>     case ARF:
>     case FIXED_GRF:
> -      inst->src[arg].reg_offset = entry->src.reg_offset;
> -      inst->src[arg].subreg_offset = entry->src.subreg_offset;
> +      {
> +         inst->src[arg].reg_offset = entry->src.reg_offset;
> +         inst->src[arg].subreg_offset = entry->src.subreg_offset;
> +
> +         /* If we copy propagate from a larger type we have to be aware that
> +          * the instruction might be using subreg_offset to select a particular
> +          * chunk of the data in the entry. For example:
> +          *
> +          * mov(8) vgrf5:DF, u2<0>:DF
> +          * mov(8) vgrf7:UD, vgrf5+0.4<2>:UD
> +          *
> +          * vgrf5+0.4<2>:UD is actually reading the high 32-bit of u2.0, so if
> +          * we want to copy propagate here we have to do it from u2+0.4.
> +          */
> +         int type_sz_src = type_sz(inst->src[arg].type);
> +         int type_sz_entry = type_sz(entry->src.type);
> +         if (type_sz_entry > type_sz_src) {
> +            inst->src[arg].subreg_offset +=
> +               inst->src[arg].subreg_offset % type_sz_entry;

Seeing 'inst->src[arg].subreg_offset' on both sides of this += seems
strange. Is this correct?

-Jordan

> +         }
> +      }
>        break;
>     case ATTR:
>     case VGRF:
> -- 
> 2.5.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev