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. 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
