I've forgot to Cc: the mailing list.
On 15/06/18 01:54, Chema Casanova wrote: > On 14/06/18 03:36, Jason Ekstrand wrote: >> On Sat, Jun 9, 2018 at 4:13 AM, Jose Maria Casanova Crespo >> <jmcasan...@igalia.com <mailto:jmcasan...@igalia.com>> wrote: >> >> shuffle_from_32bit_read manages 32-bit reads to 32-bit destination >> in the same way that the previous loop so now we just call the new >> function for all bitsizes, simplifying also the 64-bit load_input. >> --- >> 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 6abc7c0174d..fedf3bf5a83 100644 >> --- a/src/intel/compiler/brw_fs_nir.cpp >> +++ b/src/intel/compiler/brw_fs_nir.cpp >> @@ -2483,16 +2483,8 @@ fs_visitor::nir_emit_vs_intrinsic(const >> fs_builder &bld, >> if (type_sz(dest.type) == 8) >> first_component /= 2; >> >> - for (unsigned j = 0; j < num_components; j++) { >> - bld.MOV(offset(dest, bld, j), offset(src, bld, j + >> first_component)); >> - } >> - >> - if (type_sz(dest.type) == 8) { >> - shuffle_32bit_load_result_to_64bit_data(bld, >> - dest, >> - retype(dest, >> BRW_REGISTER_TYPE_F), >> - >> instr->num_components); >> - } >> + shuffle_from_32bit_read(bld, dest, retype(src, >> BRW_REGISTER_TYPE_D), >> + first_component, num_components); >> >> >> I think this is ok. It makes me a bit nervous to use >> shuffle_from_32bit_read on the address register file However, since >> we're only doing it when type_sz(dst.type) >= 4, it should be ok. > > And as I am going to implement same size shuffle to the same code of > MOVs we are removing here it would be as safe at is now. > >> If we want 16-bit attributes (Yeah, I know, I need to review that...) then we >> may need to first copy from the ATTR file into a temp. Maybe drop a >> comment to that effect? > > All the logic from "i965/fs: Unpack 16-bit from 32-bit components in VS > load_input" [1] is already implemented here with the new shuffle, so > 16-bit VS load_input changes would be already implemented with the > refactoring, (there are 4 other patches needed to solve the issues about > the vertex buffer format, padding, etc). > > [1] https://patchwork.freedesktop.org/patch/206476/ > > I'm including the following comment: > > /* For 16-bit support maybe a temporary is needed to copy from the ATTR > file */ > > I would need to find a test case that can expose this problem... > > Whenever you feel with energy for reviewing 16-bit inputs outputs let me > know and I'll send an updated/rebased version. But I'm have also pending > the review of "i965/fs: Register allocator shouldn't use grf127 for > sends dest (v2)" [2] :-) > > [2] https://patchwork.freedesktop.org/patch/217811/ > > Thanks for the review, > > Chema > >> Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net >> <mailto:ja...@jlekstrand.net>> >> >> >> break; >> } >> >> -- >> 2.17.1 >> >> _______________________________________________ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.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