[Mesa-dev] [PATCHv3 09/20] i965/fs: Get 64-bit indirect moves working on IVB.

[Samuel Iglesias Gonsálvez]

On Thu, 2017-02-09 at 18:28 -0800, Francisco Jerez wrote:
> Francisco Jerez <currojerez at riseup.net> writes:
> 
> > ---
> > This replaces "[PATCH v2 09/20] i965/fs: indirect addressing with
> > doubles is not supported in IVB/BYT".
> > 
> 
> Note that some of the fp64 indirect addressing test-cases still fail
> on
> IVB even with this patch applied, but the reason doesn't seem to have
> anything to do with indirect addressing.  Apparently the remaining
> failures are caused by illegal code like:
> 
> > sel(8)          g30<1>D         g9<0,2,1>DF     g8.3<0,2,1>DF   {
> > align1 1Q };
> 
> The problem is that the SEL instruction doesn't support most datatype
> conversions, so this leads to data corruption on at least IVB.
> According to the hardware docs, the only valid destination type of
> SEL
> is DF if the execution type is DF, and F (or HF on CHV+) if the
> execution type is F.  Integer 16 or 32-bit execution types seem to
> allow
> converting to other single-precision integer types.
> 

Some weeks ago I saw this issue while working in a solution for IVB's
MOV INDIRECT. That SEL is added by opt_peephole_sel():

https://lists.freedesktop.org/archives/mesa-dev/2017-January/142023.htm
l

However,  I did it very restrictive by just don't allowing any
conversion between different data type sizes. I am going to improve
that patch to include all the allowed cases.

> I'm not sure why we haven't seen this cause massive breakage on HSW+,
> but I think we need some sort of legalization pass to do the type
> conversion in a separate MOV for any instruction with an invalid
> destination conversion.  You may be able to do it within the d2x pass
> but then it would make sense to rename it to something more
> appropriate
> like destination conversion lowering (lower_cvt? :P).
> 

:)

> In addition there seem to be other issues with your d2x lowering code
> (I'm bringing this up here because you don't seem to have sent the
> d2x
> changes for review to the mailing list yet) -- It removes the
> original
> instruction and creates a new one with the same opcode and first few
> sources, which will miscompile the program if the original
> instruction
> had a larger number of sources or any other instruction controls
> set.  I
> suggest you modify the original instruction in-place to point it to
> the
> temporary you've allocated, and then copy the data into the original
> destination by using a strided move.
> 

OK.

Thanks,

Sam

> > src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 27
> > ++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > index ea4a3fe1399..0e2dbe23571 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > @@ -440,7 +440,7 @@ 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);
> >  
> >        /* We use VxH indirect addressing, clobbering a0.0 through
> > a0.7. */
> >        struct brw_reg addr = vec8(brw_address_reg(0));
> > @@ -478,7 +478,30 @@ fs_generator::generate_mov_indirect(fs_inst
> > *inst,
> >         * code, using it saves us 0 instructions and would require
> > quite a bit
> >         * of case-by-case work.  It's just not worth it.
> >         */
> > -      brw_ADD(p, addr, indirect_byte_offset,
> > brw_imm_uw(imm_byte_offset));
> > +      if (devinfo->gen >= 8 || devinfo->is_haswell ||
> > type_sz(reg.type) < 8) {
> > +         brw_ADD(p, addr, indirect_byte_offset,
> > brw_imm_uw(imm_byte_offset));
> > +      } else {
> > +         /* IVB reads two address register components per channel
> > for
> > +          * indirectly addressed 64-bit sources, so we need to
> > initialize
> > +          * adjacent address components to consecutive dwords of
> > the source
> > +          * region by emitting two separate ADD
> > instructions.  Found
> > +          * empirically.
> > +          */
> > +         assert(inst->exec_size <= 4);
> > +         brw_push_insn_state(p);
> > +         brw_set_default_exec_size(p, cvt(inst->exec_size) - 1);
> > +
> > +         brw_ADD(p, spread(addr, 2), indirect_byte_offset,
> > +                 brw_imm_uw(imm_byte_offset));
> > +         brw_inst_set_no_dd_clear(devinfo, brw_last_inst, true);
> > +
> > +         brw_ADD(p, spread(suboffset(addr, 1), 2),
> > indirect_byte_offset,
> > +                 brw_imm_uw(imm_byte_offset + 4));
> > +         brw_inst_set_no_dd_check(devinfo, brw_last_inst, true);
> > +
> > +         brw_pop_insn_state(p);
> > +      }
> > +
> >        struct brw_reg ind_src = brw_VxH_indirect(0, 0);
> >  
> >        brw_inst *mov = brw_MOV(p, dst, retype(ind_src, dst.type));
> > -- 
> > 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: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170210/4e210722/attachment.sig>