[Mesa-dev] [PATCH] i965/fs: Don't disable SIMD16 when using the pixel interpolator

[Chris Forbes]

Looks OK to me. I didn't think there was going to be much required to
make this work -- is nice that it turned out to be nothing.

Reviewed-by: Chris Forbes <chrisf at ijw.co.nz>

- Chris

On Fri, Jul 3, 2015 at 6:41 AM, Neil Roberts <neil at linux.intel.com> wrote:
> There was a comment saying that in SIMD16 mode the pixel interpolator
> returns coords interleaved 8 channels at a time and that this requires
> extra work to support. However, this interleaved format is exactly
> what the PLN instruction requires so I don't think anything needs to
> be done to support it apart from removing the line to disable it and
> to ensure that the message lengths for the send message are correct.
>
> I am more convinced that this is correct because as it says in the
> comment this interleaved output is identical to what is given in the
> thread payload. The code generated to apply the plane equation to
> these coordinates is identical on SIMD16 and SIMD8 except that the
> dispatch width is larger which implies no special unmangling is
> needed.
>
> Perhaps the confusion stems from the fact that the description of the
> PLN instruction in the IVB PRM seems to imply that the src1 inputs are
> not interleaved so it wouldn't work. However, in the HSW and BDW PRMs,
> the pseudo-code is different and looks like it expects the interleaved
> format. Mesa doesn't seem to generate different code on IVB to
> uninterleave the payload registers and everything is working so I can
> only assume that the PRM is wrong.
>
> I tested the interpolateAt tests on HSW and did a full Piglit run on
> IVB on there were no regressions.
> ---
>
> I've CC'd Chris Forbes because according to git-annotate he wrote the
> original comment so he might know something I don't.
>
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 59081ea..717e597 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1461,12 +1461,6 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
>     case nir_intrinsic_interp_var_at_centroid:
>     case nir_intrinsic_interp_var_at_sample:
>     case nir_intrinsic_interp_var_at_offset: {
> -      /* in SIMD16 mode, the pixel interpolator returns coords interleaved
> -       * 8 channels at a time, same as the barycentric coords presented in
> -       * the FS payload. this requires a bit of extra work to support.
> -       */
> -      no16("interpolate_at_* not yet supported in SIMD16 mode.");
> -
>        fs_reg dst_xy = bld.vgrf(BRW_REGISTER_TYPE_F, 2);
>
>        /* For most messages, we need one reg of ignored data; the hardware
> @@ -1531,7 +1525,7 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
>                             bld.SEL(offset(src, i), itemp, fs_reg(7)));
>              }
>
> -            mlen = 2;
> +            mlen = 2 * dispatch_width / 8;
>              inst = bld.emit(FS_OPCODE_INTERPOLATE_AT_PER_SLOT_OFFSET, dst_xy, src,
>                              fs_reg(0u));
>           }
> @@ -1543,7 +1537,8 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
>        }
>
>        inst->mlen = mlen;
> -      inst->regs_written = 2; /* 2 floats per slot returned */
> +      /* 2 floats per slot returned */
> +      inst->regs_written = 2 * dispatch_width / 8;
>        inst->pi_noperspective = instr->variables[0]->var->data.interpolation ==
>                                 INTERP_QUALIFIER_NOPERSPECTIVE;
>
> --
> 1.9.3
>