Re: [Mesa-dev] [PATCH 07/14] intel/compiler: shuffle_from_32bit_read for 64-bit do_untyped_vector_read
On Thu, Jun 14, 2018 at 10:57 PM, Iago Toral 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
> > > 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
> > > ,
> > >
> > > 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 > > >
> > >
> > >
> > > + 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 > > op.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 07/14] intel/compiler: shuffle_from_32bit_read for 64-bit do_untyped_vector_read
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
> > 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
> > ,
> >
> > 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 > >
> >
> >
> > + 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 > op.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 07/14] intel/compiler: shuffle_from_32bit_read for 64-bit do_untyped_vector_read
On 14/06/18 03:26, Jason Ekstrand wrote:
> On Sat, Jun 9, 2018 at 4:13 AM, Jose Maria Casanova Crespo
> 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 ,
> 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.
So maybe Iago remembers if there was any reason why...
> Reviewed-by: Jason Ekstrand >
>
>
> + 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
> 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 07/14] intel/compiler: shuffle_from_32bit_read for 64-bit do_untyped_vector_read
On Sat, Jun 9, 2018 at 4:13 AM, Jose Maria Casanova Crespo <
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 ,
> 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.
Reviewed-by: Jason Ekstrand
> + 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
> 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 07/14] intel/compiler: shuffle_from_32bit_read for 64-bit do_untyped_vector_read
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 ,
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));
- }
+ 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
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
