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

[Francisco Jerez]

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.

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.

> 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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170209/93659b1d/attachment.sig>