Re: [Mesa-dev] [PATCH 04/23] i965/fs: fix requirements to allow type change in copy-propagation
Should the subject say something about not allowing a type size change? -Jordan On 2016-05-03 05:21:53, Samuel Iglesias Gonsálvez wrote: > From: Iago Toral Quiroga > > 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. > > 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 >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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/23] i965/fs: fix requirements to allow type change in copy-propagation
On 04/05/16 01:04, Jordan Justen wrote: > Should the subject say something about not allowing a type size > change? > I don't think it is necessary but I don't have a strong opinion. What do you think about this? i965/fs: don't allow type change in copy-propagation if types have different sizes Sam > -Jordan > > On 2016-05-03 05:21:53, Samuel Iglesias Gonsálvez wrote: >> From: Iago Toral Quiroga >> >> 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. >> >> 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 >>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 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/23] i965/fs: fix requirements to allow type change in copy-propagation
Samuel Iglesias Gonsálvez writes: > From: Iago Toral Quiroga > > 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 >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
