[Mesa-dev] [PATCH 04/23] i965/fs: fix requirements to allow type change in copy-propagation

[Francisco Jerez]

Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:

> From: Iago Toral Quiroga <itoral at igalia.com>
>
> When source modifiers are present and the types of the source and
> the entry's source are different, there are certain cases in which
> we allow copy-propagation to change the type of source by the type
> of the entry's source we are copy propagating from.
>
> However, it is not generally safe to do this if the types involved
> have different sizes, since parameters like the stride would change
> the semantics of the resulting instruction.
>
AFAICT this will be a problem regardless of strides and other regioning
parameters -- The problem is that because the semantics of source
modifiers are type-dependent, the type of the original source of the
copy must be kept unmodified while propagating it into some instruction,
which implies that we need to have the guarantee that the meaning of the
instruction is going to remain the same after we've changed the types.
In particular when the size of the new type is different from the size
of the old type the new and old instructions cannot possibly be
equivalent because the new instruction will be reading more data that
the old one was.

I suggest you clarify the paragraph above and/or introduce a short
comment in the code explaining why a copy instruction with source
modifiers requires the instruction being propagated into to have a
special form.

> Prevents that we turn this:
>
> load_payload(8) vgrf17:DF, |vgrf4+0.0|:DF 1sthalf
> mov(8) vgrf18:DF, vgrf17:DF 1sthalf
> load_payload(8) vgrf5:DF, vgrf18:DF, vgrf20:DF NoMask 1sthalf WE_all
> load_payload(8) vgrf21:UD, vgrf5+0.4<2>:UD 1sthalf
> mov(8) vgrf22:UD, vgrf21:UD 1sthalf
>
> into:
>
> load_payload(8) vgrf17:DF, |vgrf4+0.0|:DF 1sthalf
> mov(8) vgrf18:DF, |vgrf4+0.0|:DF 1sthalf
> load_payload(8) vgrf5:DF, |vgrf4+0.0|:DF, |vgrf4+2.0|:DF NoMask 1sthalf WE_all
> load_payload(8) vgrf21:UD, vgrf5+0.4<2>:UD 1sthalf
> mov(8) vgrf22:DF, |vgrf4+0.4|<2>:DF 1sthalf
>
> where the semantics of the last instruccion have changed.
> ---
>  src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 5 +++--
>  1 file changed, 3 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 abc68c8..aa4c9c9 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -411,8 +411,9 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>        return false;
>  
>     if (has_source_modifiers &&
> -       entry->dst.type != inst->src[arg].type &&
> -       !inst->can_change_types())
> +       ((entry->dst.type != inst->src[arg].type &&
> +         !inst->can_change_types()) ||
> +        (type_sz(entry->dst.type) != type_sz(inst->src[arg].type))))

I would find the expression above more readable (less parentheses
overall) if you did something like:

| (has_source_modifiers &&
|  entry->dst.type != inst->src[arg].type &&
|  (!inst->can_change_types() ||
|   type_sz(entry->dst.type) != type_sz(inst->src[arg].type)))

Either way looks correct to me:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

>        return false;
>  
>     if (devinfo->gen >= 8 && (entry->src.negate || entry->src.abs) &&
> -- 
> 2.5.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160510/b1a55293/attachment.sig>