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