<div dir="ltr"><div><div><div><div>On 19 December 2013 14:52, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>

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