On Tue, 2016-05-03 at 16:21 -0700, Jordan Justen wrote:
> On 2016-05-03 05:21:54, Samuel Iglesias Gonsálvez wrote:
> > From: Iago Toral Quiroga <ito...@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?
Yes, this looks wrong. I'll fix it, thanks!
> -Jordan
>
> > + }
> > + }
> > break;
> > case ATTR:
> > case VGRF:
> > --
> > 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
