[Mesa-dev] [PATCH 08/23] i965/fs: Add support for specifying register horizontal strides.

[Paul Berry]

On 19 December 2013 14:52, Francisco Jerez <currojerez at riseup.net> wrote:

> Paul Berry <stereotype441 at gmail.com> writes:
>
> >[...]
> > In v2 of this patch, you add the following code to
> > fs_visitor::try_copy_propagate():
> >
> > +   /* Bail if the result of composing both strides cannot be expressed
> > +    * as another stride.
> > +    */
> > +   if (entry->src.stride != 1 &&
> > +       (inst->src[arg].stride *
> > +        type_sz(inst->src[arg].type)) % type_sz(entry->src.type) != 0)
> >        return false;
> >
> > I don't understand.  It seems like this is trying to make sure that
> > (src_stride * src_type_sz) / entry_type_sz is an integer so we can later
> > use the factor (src_type_sz / entry_type_sz) as a multiplicative factor
> to
> > correct the stride without creating a fractional result.  But I don't see
> > us applying this correction factor anywhere.  Is there some code missing?
>
> That isn't exactly the purpose of that check.
>
> The problem arises when you are trying to compose two regions of
> different base types.  If the stride in bytes of 'inst->src[arg]' is not
> a multiple of the element size of 'entry->src' you end up with an
> inhomogeneous stride that can only be expressed as a two-dimensional
> region.  Instead of adding support for two-dimensional regions (which
> would be hard, not very useful, and only a partial solution because then
> we would need to handle composition of two-dimensional regions that
> yield three- and four-dimensional regions as a result) I disallowed
> copy-propagation in that case.
>
> Thanks.
>

Ok, I see.  I had to spend quite a while fiddling with scratch paper to
understand this.  Would you mind adding an example to the comment to
clarify this?  Perhaps something like this:

   /* Bail if the result of composing both strides cannot be expressed as
    * another stride.  This avoids, for example, trying to transform this:
    *
    *     MOV (8) rX<1>UD rY<0;1,0>UD
    *     FOO (8) ...     rX<8;8,2>UW
    *
    * into this:
    *
    *     FOO (8) ...     rY<8;8,0>UW
    *
    * Which would have different semantics.
    */

Note: the code sequence in my example is bogus because it crosses data
between SIMD execution paths, so it would never actually arise in
practice.  I wasn't able to come up with a realistic example; in fact,
after trying and failing to come up with a non-bogus example, I'm pretty
convinced that this particular bailout case will never happen unless
there's a bug elsewhere in the compiler.  You might want to consider adding
an assert(false) into the code path so that if it ever does occur we'll
have a chance to investigate.

I don't have strong feelings about adding the assert, though.  With the
comment updated (and the other comment suggestion from my previous email
about is_power_of_two()), this patch is:

Reviwed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131230/256a46ae/attachment-0001.html>