[Mesa-dev] [PATCH 09/23] i965/fs: don't copy propagate from a larger type if the stride is not 1

[Iago Toral]

On Tue, 2016-05-10 at 17:28 -0700, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
> > From: Iago Toral Quiroga <itoral at igalia.com>
> >
> > Because the stride is in units for the type, if we copy-propagate from
> > a another instruction using a larger type, then we need to make sure
> > that the source in that instruction, the one we will be copy-propagating
> > from, sources consecutive elements, otherwise, when sourced using a
> > a smaller type, the actual elements read would change.
> >
> > Prevents that we turn this:
> > mov(8) vgrf3+2.0:DF, vgrf11<0>:DF
> > load_payload(8) vgrf15:UD, ..., vgrf3+2.0<0>:D, vgrf3+3.0<0>:D
> >
> > Into:
> > mov(8) vgrf3+2.0:DF, vgrf11<0>:DF
> > load_payload(8) vgrf15:UD, ..., vgrf11<0>:D, vgrf11<0>:D
> >
> 
> Sorry but I don't see the problem, the two assembly examples look fully
> equivalent to me.

Right, I copied the wrong chunk when writing the commit log. The problem
I was trying to address with this was related to your last comment to
patch 5, which was still not computing the right subreg_offset in all
scenarios. Basically, I had something like:

mov(8) vgrf4+0.0:UD vgrf69+0.4<2>:UD
and(8) vgrf4+0.0:UD vgrf4+0.0:UD 2147483648u

And copy propagation was turning it into:

mov(8) vgrf4+0.0:UD u0+0.4<0>:UD
and(8) vgrf4+0.0:UD u0<0>:UD 2147483648u

So it is basically the subreg_offset calculation that was wrong (which
the 2-patch series you attached in reply to patch 5 fix).

In retrospect, I am not sure how I came up with this patch because it
does not even try to address the actual problem, it simply prevents
copy-propagation from acting on the first instruction, which then
prevents the bug to show up in the second one so it simply fixed the
problem by accident. I don't know what I was thinking :-/

With your two patches the problem is fixed so we can just drop this
patch.

> > In the original instructions, vgrf3+2.0<0>:D reads a replicated 64-bit
> > value, while the result after copy propagation only reads the first
> > 32-bit of the value.
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > 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 9fc06cb..f98ab41 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> > @@ -400,6 +400,15 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
> >     if (type_sz(entry->dst.type) < type_sz(inst->src[arg].type))
> >        return false;
> >  
> > +   /* Bail if the instruction type is larger than the execution type of the
> > +    * copy and the stride of the source we would be copy propagating from
> > +    * has a stride other than 1. Otherwise, since the stride is in units of
> > +    * the type, we would be changing the region effectively sourced.
> > +    */
> > +   if (type_sz(entry->dst.type) > type_sz(inst->src[arg].type) &&
> > +       entry->src.stride != 1)
> > +      return false;
> 
> NAK, there is a more accurate condition just a few lines below making
> sure that the strides can be composed correctly when the original
> entry->src.stride is not one.  Some special cases of
> 'type_sz(entry->dst.type) > type_sz(inst->src[arg].type) &&
> entry->src.stride != 1' are handled by copy propagation, and the cases
> that are not should already be caught by the check immediately below.
> 
> > +
> >     /* Bail if the result of composing both strides cannot be expressed
> >      * as another stride. This avoids, for example, trying to transform
> >      * this:
> > -- 
> > 2.5.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev