Re: [Mesa-dev] [PATCH 09/14] intel/compiler: Use shuffle_from_32bit_read at VS load_input
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
>> 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 ,
>> 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 > >
>>
>>
>> break;
>> }
>>
>> --
>> 2.17.1
>>
>> ___
>> 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
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 09/14] intel/compiler: Use shuffle_from_32bit_read at VS load_input
On Sat, Jun 9, 2018 at 4:13 AM, Jose Maria Casanova Crespo <
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
> ,
>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. 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?
Reviewed-by: Jason Ekstrand
>break;
> }
>
> --
> 2.17.1
>
> ___
> 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
[Mesa-dev] [PATCH 09/14] intel/compiler: Use shuffle_from_32bit_read at VS load_input
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 ,
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);
break;
}
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
