[Mesa-dev] [PATCH v2 09/20] i965/fs: indirect addressing with doubles is not supported in IVB/BYT

Francisco Jerez currojerez at riseup.net
Fri Feb 10 18:10:20 UTC 2017


Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:

> On Thu, 2017-02-09 at 12:18 -0800, Francisco Jerez wrote:
>> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
>> 
>> > It is tested empirically that IVB/BYT don't support indirect
>> > addressing
>> > with doubles but it is not documented in the PRM.
>> > 
>> > This patch applies the same solution than for Cherryview/Broxton
>> > and
>> > takes into account that we cannot double the stride, since the
>> > hardware will do it internally.
>> > 
>> > v2:
>> > - Fix assert to take into account Indirect DF MOVs in IVB and HSW.
>> > 
>> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 27
>> > ++++++++++++++++++--------
>> >  src/mesa/drivers/dri/i965/brw_fs_nir.cpp       | 11 ++++++-----
>> >  2 files changed, 25 insertions(+), 13 deletions(-)
>> > 
>> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> > b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> > index 487f2e90224..dd6cbab773c 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> > @@ -418,18 +418,29 @@ fs_generator::generate_mov_indirect(fs_inst
>> > *inst,
>> >        brw_MOV(p, dst, reg);
>> >     } else {
>> >        /* Prior to Broadwell, there are only 8 address registers.
>> > */
>> > -      assert(inst->exec_size == 8 || devinfo->gen >= 8);
>> > +      assert(inst->exec_size == 8 || devinfo->gen >= 8 ||
>> > +             (devinfo->gen == 7 && inst->exec_size < 8 &&
>> > +              (type_sz(reg.type) == 8 || type_sz(dst.type) ==
>> > 8)));
>> >  
>> >        /* We use VxH indirect addressing, clobbering a0.0 through
>> > a0.7. */
>> >        struct brw_reg addr = vec8(brw_address_reg(0));
>> >  
>> > -      /* The destination stride of an instruction (in bytes) must
>> > be greater
>> > -       * than or equal to the size of the rest of the
>> > instruction.  Since the
>> > -       * address register is of type UW, we can't use a D-type
>> > instruction.
>> > -       * In order to get around this, re retype to UW and use a
>> > stride.
>> > -       */
>> > -      indirect_byte_offset =
>> > -         retype(spread(indirect_byte_offset, 2),
>> > BRW_REGISTER_TYPE_UW);
>> > +      if (devinfo->gen != 7 || devinfo->is_haswell ||
>> > type_sz(reg.type) != 8) {
>> > +         /* The destination stride of an instruction (in bytes)
>> > must be greater
>> > +          * than or equal to the size of the rest of the
>> > instruction.  Since the
>> > +          * address register is of type UW, we can't use a D-type
>> > instruction.
>> > +          * In order to get around this, re retype to UW and use a
>> > stride.
>> > +          */
>> > +         indirect_byte_offset =
>> > +            retype(spread(indirect_byte_offset, 2),
>> > BRW_REGISTER_TYPE_UW);
>> > +      } else {
>> > +         /* In Ivybridge/Baytrail, when it operates with DF
>> > operands, we
>> > +          * cannot double the stride, since the hardware will do
>> > it
>> > +          * internally. Tested empirically.
>> > +          */
>> > +         indirect_byte_offset =
>> > +            retype(indirect_byte_offset, BRW_REGISTER_TYPE_UW);
>> > +      }
>> 
>> This doesn't make any sense, indirect_byte_offset is a 32-bit integer
>> type regardless of the type of the data you're copying, the hardware
>> won't do any stride doubling for you here.
>> 
>
> Yeah, I realised this was wrong when looking at the bug mentioned by
> Matt. Hence my work to provide a proper solution, which we discussed
> privately.
>
> I've verified on IVB hardware that indirect addressing works for 64-
> bit
>> moves, but getting it to do what you want is a bit trickier than on
>> more
>> recent hardware -- I'll reply with a patch that gets it working.
>> 
>
> OK
>
>> >  
>> >        /* There are a number of reasons why we don't use the base
>> > offset here.
>> >         * One reason is that the field is only 9 bits which means
>> > we can only
>> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> > index 8f745dff440..85f43b7b144 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> > @@ -3822,17 +3822,18 @@ fs_visitor::nir_emit_intrinsic(const
>> > fs_builder &bld, nir_intrinsic_instr *instr
>> >              (instr->num_components - 1) * type_sz(dest.type);
>> >  
>> >           fs_reg indirect_chv_high_32bit;
>> > -         bool is_chv_bxt_64bit =
>> > -            (devinfo->is_cherryview || devinfo->is_broxton) &&
>> > -            type_sz(dest.type) == 8;
>> > -         if (is_chv_bxt_64bit) {
>> > +         bool is_ivb_byt_chv_bxt_64bit =
>> 
>> Let's use a more sensible name here like "supports_64bit_indirects"
>> and
>> mark as const...  Or just remove the variable altogether and fold
>> this
>> into the condition of the second if statement below.
>> 
>
> OK
>
>> > +            (devinfo->is_cherryview || devinfo->is_broxton ||
>> > +             devinfo->is_ivybridge || devinfo->is_baytrail) &&
>> > +           type_sz(dest.type) == 8;
>> > +         if (is_ivb_byt_chv_bxt_64bit) {
>> >              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));
>> 
>> The ADD and temporary seem redundant, you could take into account the
>> additional 32bit offset by applying an immediate offset to the 'src'
>> region used below.
>> 
>> >           }
>> >  
>> >           for (unsigned j = 0; j < instr->num_components; j++) {
>> > -            if (!is_chv_bxt_64bit) {
>> > +            if (!is_ivb_byt_chv_bxt_64bit) {
>> 
>> The code below seems pretty broken...  You lower a 64-bit to 64-bit
>> indirect move into two 64-bit to UD type-converting moves, which is
>> unlikely to do what you want.
>>   You need to lower it into two raw 32-bit
>> to 32bit moves (e.g. by using subscript() in the source as well as on
>> the destination).  Please fix this (and CC the patch to mesa-stable)
>> regardless of my patch to get 64-bit MOV_INDIRECT working, CHV/BXT
>> are
>> unlikely to work as expected except by luck.
>> 
>
> This is likely working because in fs_generator::generate_mov_indirect()
>  the source of the MOV is retyped to destination's type (at this case
> an UD). So, at the end, there is no generation of a 64-bit to UD
> converting move.
>

Yeah, that likely prevented things from failing catastrophically [this
is what I call luck :P], but generate_mov_indirect() dropping the source
type on the floor likely qualifies as a bug in its own right...

> But you are right, this is wrong. I'll fix it.
>
> Thanks for the feedback!
>
> Sam
>
>> >                 bld.emit(SHADER_OPCODE_MOV_INDIRECT,
>> >                          offset(dest, bld, j), offset(src, bld, j),
>> >                          indirect, brw_imm_ud(read_size));
>> > -- 
>> > 2.11.0
>> > 
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170210/e60011c1/attachment.sig>


More information about the mesa-dev mailing list