[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 <siglesias at igalia.com>
> > Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> > 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 &bld, 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 <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160617/a1f01dcd/attachment.sig>