Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes: > From: Iago Toral Quiroga <ito...@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 <curroje...@riseup.net> > return false; > > if (devinfo->gen >= 8 && (entry->src.negate || entry->src.abs) && > -- > 2.5.0 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
