<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Nov 6, 2017 at 4:10 AM, Samuel Iglesias Gonsálvez <span dir="ltr"><<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
On Thu, 2017-11-02 at 15:54 -0700, Jason Ekstrand wrote:<br>
> Register strides higher than 4 are uncommon but they can happen.  For<br>
> instance, if you have a 64-bit extract_u8 operation, we turn that<br>
> into<br>
> UB -> UQ MOV with a source stride of 8.  Our previous calculation<br>
> would<br>
> try to generate a stride of <32;8,8>:ub which is invalid because the<br>
> maximum horizontal stride is 4.  To solve this problem, we instead<br>
> use a<br>
> stride of <8;1,0>.  As noted in the comment, this does not work as a<br>
> destination but that's ok as very few things actually generate that<br>
> stride.<br>
><br>
<br>
</span>Great!<br>
<div><div class="h5"><br>
> Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.<wbr>org</a><br>
> ---<br>
>  src/intel/compiler/brw_fs_<wbr>generator.cpp | 15 ++++++++++++---<br>
>  1 file changed, 12 insertions(+), 3 deletions(-)<br>
><br>
> diff --git a/src/intel/compiler/brw_fs_<wbr>generator.cpp<br>
> b/src/intel/compiler/brw_fs_<wbr>generator.cpp<br>
> index 46f9a33..a557f80 100644<br>
> --- a/src/intel/compiler/brw_fs_<wbr>generator.cpp<br>
> +++ b/src/intel/compiler/brw_fs_<wbr>generator.cpp<br>
> @@ -90,9 +90,18 @@ brw_reg_from_fs_reg(const struct gen_device_info<br>
> *devinfo, fs_inst *inst,<br>
>            *       different execution size when the number of<br>
> components<br>
>            *       written to each destination GRF is not the same.<br>
>            */<br>
> -         const unsigned width = MIN2(reg_width, phys_width);<br>
> -         brw_reg = brw_vecn_reg(width, brw_file_from_reg(reg), reg-<br>
> >nr, 0);<br>
> -         brw_reg = stride(brw_reg, width * reg->stride, width, reg-<br>
> >stride);<br>
> +         if (reg->stride > 4) {<br>
> +            /* For registers with an exceptionally large stride, we<br>
> use a<br>
> +             * width of 1 and only use the vertical stride.  This<br>
> only works<br>
> +             * for sources since destinations require hstride == 1.<br>
> +             */<br>
> +            brw_reg = brw_vec1_reg(brw_file_from_<wbr>reg(reg), reg->nr,<br>
> 0);<br>
> +            brw_reg = stride(brw_reg, reg->stride, 1, 0);<br>
<br>
</div></div>I think it is a good idea to add an assert like:<br>
<br>
   assert(reg != &inst->dst)<br>
<br>
in order to avoid applying this to dst.<br></blockquote><div><br></div><div>We already have an assert that triggers, but this is more direct.  I'll add it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
With or without that change,<br>
<br>
Reviewed-by: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com">siglesias@igalia.com</a>><br></blockquote><div><br></div><div>Thanks!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Sam<br>
<div class="HOEnZb"><div class="h5"><br>
> +         } else {<br>
> +            const unsigned width = MIN2(reg_width, phys_width);<br>
> +            brw_reg = brw_vecn_reg(width, brw_file_from_reg(reg),<br>
> reg->nr, 0);<br>
> +            brw_reg = stride(brw_reg, width * reg->stride, width,<br>
> reg->stride);<br>
> +         }<br>
><br>
>           if (devinfo->gen == 7 && !devinfo->is_haswell) {<br>
>              /* From the IvyBridge PRM (EU Changes by Processor<br>
> Generation, page 13):</div></div></blockquote></div><br></div></div>