Re: [Mesa-dev] [PATCH v3] i965/fs: indirect addressing with doubles is not supported in CHV/BSW/BXT

[Samuel Iglesias Gonsálvez]

On 17/06/16 11:12, Kenneth Graunke wrote:
> On Friday, June 17, 2016 11:10:28 AM PDT Samuel Iglesias Gonsálvez wrote:
[...]
>> What do you think Kenneth?
>>
>> Sam
> 
> This sounds great to me.  I like v4 (your suggestion above) the best.
> Thanks for fixing this, and putting in the extra effort to simplify
> things!
> 
> Either v3 or v4 are
> Reviewed-by: Kenneth Graunke 
> 

Thanks a lot for the review! :-)

Sam



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3] i965/fs: indirect addressing with doubles is not supported in CHV/BSW/BXT

[Kenneth Graunke]

On Friday, June 17, 2016 11:10:28 AM PDT Samuel Iglesias Gonsálvez wrote:
> 
> On 17/06/16 10:43, Samuel Iglesias Gonsálvez wrote:
> > From the Cherryview's PRM, Volume 7, 3D Media GPGPU Engine, Register Region
> > Restrictions, page 844:
> > 
> >   "When source or destination datatype is 64b or operation is integer DWord
> >multiply, indirect addressing must not be used."
> > 
> > v2:
> > - Fix it for Broxton too.
> > 
> > v3:
> > - Simplify code by using subscript() and not creating a new num_components
> > variable (Kenneth).
> > 
> > Signed-off-by: Samuel Iglesias Gonsálvez 
> > Cc: "12.0" 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95462
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 20 +++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
> > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > index d72b37b..20db075 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > @@ -3611,10 +3611,28 @@ fs_visitor::nir_emit_intrinsic(const fs_builder 
> > , nir_intrinsic_instr *instr
> >   unsigned read_size = instr->const_index[1] -
> >  (instr->num_components - 1) * type_sz(dest.type);
> >  
> > + fs_reg indirect_chv_high_32bit;
> > + brw_reg_type type = dest.type;
> > + bool is_chv_bxt_64bit =
> > +(devinfo->is_cherryview || devinfo->is_broxton) &&
> > +type_sz(dest.type) == 8;
> > + if (is_chv_bxt_64bit) {
> > +type = BRW_REGISTER_TYPE_UD;
> > +indirect_chv_high_32bit = vgrf(glsl_type::uint_type);
> > +/* Calculate indirect address to read high 32 bits */
> > +bld.ADD(indirect_chv_high_32bit, indirect, brw_imm_ud(4));
> > + }
> > +
> >   for (unsigned j = 0; j < instr->num_components; j++) {
> >  bld.emit(SHADER_OPCODE_MOV_INDIRECT,
> > - offset(dest, bld, j), offset(src, bld, j),
> > + subscript(offset(dest, bld, j), type, 0), offset(src, 
> > bld, j),
> >   indirect, brw_imm_ud(read_size));
> > +
> > +if (is_chv_bxt_64bit) {
> > +   bld.emit(SHADER_OPCODE_MOV_INDIRECT,
> > +subscript(offset(dest, bld, j), type, 1), 
> > offset(src, bld, j),
> > +indirect_chv_high_32bit, brw_imm_ud(read_size));
> > +}
> >   }
> 
> Although this code works fine, after talking with Kenneth in IRC, I am
> going to refactor it to put it clearer:
> 
> if (!is_chv_bxt_64bit) {
>bld.emit(SHADER_OPCODE_MOV_INDIRECT,
> offset(dest, bld, j), offset(src, bld, j),
> indirect, brw_imm_ud(read_size));
> } else {
>   bld.emit(SHADER_OPCODE_MOV_INDIRECT,
>   subscript(offset(dest, bld, j), BRW_REGISTER_TYPE_UD, 0),
>   offset(src, bld, j),
>   indirect, brw_imm_ud(read_size));
> 
>   bld.emit(SHADER_OPCODE_MOV_INDIRECT,
>subscript(offset(dest, bld, j), BRW_REGISTER_TYPE_UD, 1),
>offset(src, bld, j),
>indirect_chv_high_32bit, brw_imm_ud(read_size));
> }
> 
> And remove 'type' variable.
> 
> What do you think Kenneth?
> 
> Sam

This sounds great to me.  I like v4 (your suggestion above) the best.
Thanks for fixing this, and putting in the extra effort to simplify
things!

Either v3 or v4 are
Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3] i965/fs: indirect addressing with doubles is not supported in CHV/BSW/BXT

[Samuel Iglesias Gonsálvez]

On 17/06/16 10:43, Samuel Iglesias Gonsálvez wrote:
> From the Cherryview's PRM, Volume 7, 3D Media GPGPU Engine, Register Region
> Restrictions, page 844:
> 
>   "When source or destination datatype is 64b or operation is integer DWord
>multiply, indirect addressing must not be used."
> 
> v2:
> - Fix it for Broxton too.
> 
> v3:
> - Simplify code by using subscript() and not creating a new num_components
> variable (Kenneth).
> 
> Signed-off-by: Samuel Iglesias Gonsálvez 
> Cc: "12.0" 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95462
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index d72b37b..20db075 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -3611,10 +3611,28 @@ fs_visitor::nir_emit_intrinsic(const fs_builder , 
> nir_intrinsic_instr *instr
>   unsigned read_size = instr->const_index[1] -
>  (instr->num_components - 1) * type_sz(dest.type);
>  
> + fs_reg indirect_chv_high_32bit;
> + brw_reg_type type = dest.type;
> + bool is_chv_bxt_64bit =
> +(devinfo->is_cherryview || devinfo->is_broxton) &&
> +type_sz(dest.type) == 8;
> + if (is_chv_bxt_64bit) {
> +type = BRW_REGISTER_TYPE_UD;
> +indirect_chv_high_32bit = vgrf(glsl_type::uint_type);
> +/* Calculate indirect address to read high 32 bits */
> +bld.ADD(indirect_chv_high_32bit, indirect, brw_imm_ud(4));
> + }
> +
>   for (unsigned j = 0; j < instr->num_components; j++) {
>  bld.emit(SHADER_OPCODE_MOV_INDIRECT,
> - offset(dest, bld, j), offset(src, bld, j),
> + subscript(offset(dest, bld, j), type, 0), offset(src, 
> bld, j),
>   indirect, brw_imm_ud(read_size));
> +
> +if (is_chv_bxt_64bit) {
> +   bld.emit(SHADER_OPCODE_MOV_INDIRECT,
> +subscript(offset(dest, bld, j), type, 1), 
> offset(src, bld, j),
> +indirect_chv_high_32bit, brw_imm_ud(read_size));
> +}
>   }

Although this code works fine, after talking with Kenneth in IRC, I am
going to refactor it to put it clearer:

if (!is_chv_bxt_64bit) {
   bld.emit(SHADER_OPCODE_MOV_INDIRECT,
offset(dest, bld, j), offset(src, bld, j),
indirect, brw_imm_ud(read_size));
} else {
  bld.emit(SHADER_OPCODE_MOV_INDIRECT,
  subscript(offset(dest, bld, j), BRW_REGISTER_TYPE_UD, 0),
  offset(src, bld, j),
  indirect, brw_imm_ud(read_size));

  bld.emit(SHADER_OPCODE_MOV_INDIRECT,
   subscript(offset(dest, bld, j), BRW_REGISTER_TYPE_UD, 1),
   offset(src, bld, j),
   indirect_chv_high_32bit, brw_imm_ud(read_size));
}

And remove 'type' variable.

What do you think Kenneth?

Sam

>}
>break;
> 



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v3] i965/fs: indirect addressing with doubles is not supported in CHV/BSW/BXT

[Samuel Iglesias Gonsálvez]

From the Cherryview's PRM, Volume 7, 3D Media GPGPU Engine, Register Region
Restrictions, page 844:

  "When source or destination datatype is 64b or operation is integer DWord
   multiply, indirect addressing must not be used."

v2:
- Fix it for Broxton too.

v3:
- Simplify code by using subscript() and not creating a new num_components
variable (Kenneth).

Signed-off-by: Samuel Iglesias Gonsálvez 
Cc: "12.0" 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95462
---
 src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
index d72b37b..20db075 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
@@ -3611,10 +3611,28 @@ fs_visitor::nir_emit_intrinsic(const fs_builder , 
nir_intrinsic_instr *instr
  unsigned read_size = instr->const_index[1] -
 (instr->num_components - 1) * type_sz(dest.type);
 
+ fs_reg indirect_chv_high_32bit;
+ brw_reg_type type = dest.type;
+ bool is_chv_bxt_64bit =
+(devinfo->is_cherryview || devinfo->is_broxton) &&
+type_sz(dest.type) == 8;
+ if (is_chv_bxt_64bit) {
+type = BRW_REGISTER_TYPE_UD;
+indirect_chv_high_32bit = vgrf(glsl_type::uint_type);
+/* Calculate indirect address to read high 32 bits */
+bld.ADD(indirect_chv_high_32bit, indirect, brw_imm_ud(4));
+ }
+
  for (unsigned j = 0; j < instr->num_components; j++) {
 bld.emit(SHADER_OPCODE_MOV_INDIRECT,
- offset(dest, bld, j), offset(src, bld, j),
+ subscript(offset(dest, bld, j), type, 0), offset(src, 
bld, j),
  indirect, brw_imm_ud(read_size));
+
+if (is_chv_bxt_64bit) {
+   bld.emit(SHADER_OPCODE_MOV_INDIRECT,
+subscript(offset(dest, bld, j), type, 1), offset(src, 
bld, j),
+indirect_chv_high_32bit, brw_imm_ud(read_size));
+}
  }
   }
   break;
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev