[Mesa-dev] [PATCH 09/14] intel/compiler: Use shuffle_from_32bit_read at VS load_input

[Chema Casanova]

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
>> <jmcasanova at igalia.com <mailto:jmcasanova at 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 <jason at jlekstrand.net
>> <mailto:jason at jlekstrand.net>>
>>  
>>
>>            break;
>>         }
>>      
>>     -- 
>>     2.17.1
>>
>>     _______________________________________________
>>     mesa-dev mailing list
>>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>