<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 14, 2018 at 10:57 PM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, 2018-06-15 at 00:20 +0200, Chema Casanova wrote:<br>
> <br>
> On 14/06/18 03:26, Jason Ekstrand wrote:<br>
> > On Sat, Jun 9, 2018 at 4:13 AM, Jose Maria Casanova Crespo<br>
> > <<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a> <mailto:<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a>><wbr>> wrote:<br>
> > <br>
> >     do_untyped_vector_read is used at load_ssbo and load_shared.<br>
> > <br>
> >     The previous MOVs are removed because shuffle_from_32bit_read<br>
> >     can handle storing the shuffle results in the expected<br>
> > destination<br>
> >     just using the proper offset.<br>
> >     ---<br>
> >      src/intel/compiler/brw_fs_nir.<wbr>cpp | 12 ++----------<br>
> >      1 file changed, 2 insertions(+), 10 deletions(-)<br>
> > <br>
> >     diff --git a/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
> >     b/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
> >     index 7e738ade82e..780a9e228de 100644<br>
> >     --- a/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
> >     +++ b/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
> >     @@ -2434,16 +2434,8 @@ do_untyped_vector_read(const fs_builder<br>
> > &bld,<br>
> >                                                    <br>
> >  BRW_PREDICATE_NONE);<br>
> > <br>
> >               /* Shuffle the 32-bit load result into valid 64-bit<br>
> > data */<br>
> >     -         const fs_reg packed_result = bld.vgrf(dest.type,<br>
> >     iter_components);<br>
> >     -         shuffle_32bit_load_result_to_<wbr>64bit_data(<br>
> >     -            bld, packed_result, read_result, iter_components);<br>
> >     -<br>
> >     -         /* Move each component to its destination */<br>
> >     -         read_result = retype(read_result,<br>
> > BRW_REGISTER_TYPE_DF);<br>
> >     -         for (int c = 0; c < iter_components; c++) {<br>
> >     -            bld.MOV(offset(dest, bld, it * 2 + c),<br>
> >     -                    offset(packed_result, bld, c));<br>
> >     -         }<br>
> > <br>
> > <br>
> > I really don't know why we needed this extra set of MOVs.  They<br>
> > seem<br>
> > pretty pointless to me.  Maybe history?  In any case, this looks<br>
> > good.v-<br>
> <br>
> I've just checked and there is not much history as the 64-bit code of<br>
> this function hasn't been changed since they landed. I think that the<br>
> logic was first shuffle and then move to the proper destination<br>
> instead<br>
> of just shuffling to the final destination directly.<br>
<br>
</div></div>Could it be related to non-uniform control flow? Does the function<br>
below disable channel masks to shuffle? If it does, then I think we<br>
need to do the shuffle to a temporary and the move from there to its<br>
original destination with channel masking enabled.<span class="HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div>That could be.  I don't think the function below has any masking problems though.</div><div><br></div><div>--Jason</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888">
Iago<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> So maybe Iago remembers if there was any reason why...<br>
> <br>
> > Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a><br>
> > <mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>>><br>
> >  <br>
> > <br>
> >     +         shuffle_from_32bit_read(bld, offset(dest, bld, it *<br>
> > 2),<br>
> >     +                                 read_result, 0,<br>
> > iter_components);<br>
> > <br>
> >               bld.ADD(read_offset, read_offset, brw_imm_ud(16));<br>
> >            }<br>
> >     -- <br>
> >     2.17.1<br>
> > <br>
> >     ______________________________<wbr>_________________<br>
> >     mesa-dev mailing list<br>
> >     <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <mailto:<a href="mailto:mesa-dev@lists.freedeskt">mesa-dev@lists.<wbr>freedeskt</a><br>
> > <a href="http://op.org" rel="noreferrer" target="_blank">op.org</a>><br>
> >     <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
> >     <<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.<wbr>org/mailman/listinfo/mesa-dev</a>><br>
> > <br>
> > <br>
> > <br>
> > <br>
> > ______________________________<wbr>_________________<br>
> > mesa-dev mailing list<br>
> > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
> > <br>
> <br>
> <br>
</div></div></blockquote></div><br></div></div>