[Mesa-dev] [PATCH 1/2] i965/fs: Add/use functions to convert to 3src_align1 vstride/hstride

[Scott D Phillips]

Matt Turner <mattst88 at gmail.com> writes:

> Some cases weren't handled, such as stride 4 which is needed for 64-bit
> operations. Presumably fixes the assertion failure mentioned in commit
> 2d0457203871 (Revert "i965/fs: Use align1 mode on ternary instructions
> on Gen10+") but who can really say since the commit neglected to list
> any of them!
> ---
>  src/intel/compiler/brw_eu_emit.c | 69 ++++++++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 28 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c
> index 85bb6a4cdd..c25d8d6eda 100644
> --- a/src/intel/compiler/brw_eu_emit.c
> +++ b/src/intel/compiler/brw_eu_emit.c
> @@ -673,6 +673,42 @@ get_3src_subreg_nr(struct brw_reg reg)
>     return reg.subnr / 4;
>  }
>  
> +static enum gen10_align1_3src_vertical_stride
> +to_3src_align1_vstride(enum brw_vertical_stride vstride)
> +{
> +   switch (vstride) {
> +   case BRW_VERTICAL_STRIDE_0:
> +      return BRW_ALIGN1_3SRC_VERTICAL_STRIDE_0;
> +   case BRW_VERTICAL_STRIDE_2:
> +      return BRW_ALIGN1_3SRC_VERTICAL_STRIDE_2;
> +   case BRW_VERTICAL_STRIDE_4:
> +      return BRW_ALIGN1_3SRC_VERTICAL_STRIDE_4;
> +   case BRW_VERTICAL_STRIDE_8:
> +   case BRW_VERTICAL_STRIDE_16:
> +      return BRW_ALIGN1_3SRC_VERTICAL_STRIDE_8;

What is the reasoning for vstride 16 to map to 8 here? Could that cause
problems?

> +   default:
> +      unreachable("invalid vstride");
> +   }
> +}
> +
> +
> +static enum gen10_align1_3src_src_horizontal_stride
> +to_3src_align1_hstride(enum brw_horizontal_stride hstride)
> +{
> +   switch (hstride) {
> +   case BRW_HORIZONTAL_STRIDE_0:
> +      return BRW_ALIGN1_3SRC_SRC_HORIZONTAL_STRIDE_0;
> +   case BRW_HORIZONTAL_STRIDE_1:
> +      return BRW_ALIGN1_3SRC_SRC_HORIZONTAL_STRIDE_1;
> +   case BRW_HORIZONTAL_STRIDE_2:
> +      return BRW_ALIGN1_3SRC_SRC_HORIZONTAL_STRIDE_2;
> +   case BRW_HORIZONTAL_STRIDE_4:
> +      return BRW_ALIGN1_3SRC_SRC_HORIZONTAL_STRIDE_4;
> +   default:
> +      unreachable("invalid hstride");
> +   }
> +}
> +
>  static brw_inst *
>  brw_alu3(struct brw_codegen *p, unsigned opcode, struct brw_reg dest,
>           struct brw_reg src0, struct brw_reg src1, struct brw_reg src2)
> @@ -721,41 +757,18 @@ brw_alu3(struct brw_codegen *p, unsigned opcode, struct brw_reg dest,
>        brw_inst_set_3src_a1_src1_type(devinfo, inst, src1.type);
>        brw_inst_set_3src_a1_src2_type(devinfo, inst, src2.type);
>  
> -      assert((src0.vstride == BRW_VERTICAL_STRIDE_0 &&
> -              src0.hstride == BRW_HORIZONTAL_STRIDE_0) ||
> -             (src0.vstride == BRW_VERTICAL_STRIDE_8 &&
> -              src0.hstride == BRW_HORIZONTAL_STRIDE_1));
> -      assert((src1.vstride == BRW_VERTICAL_STRIDE_0 &&
> -              src1.hstride == BRW_HORIZONTAL_STRIDE_0) ||
> -             (src1.vstride == BRW_VERTICAL_STRIDE_8 &&
> -              src1.hstride == BRW_HORIZONTAL_STRIDE_1));
> -      assert((src2.vstride == BRW_VERTICAL_STRIDE_0 &&
> -              src2.hstride == BRW_HORIZONTAL_STRIDE_0) ||
> -             (src2.vstride == BRW_VERTICAL_STRIDE_8 &&
> -              src2.hstride == BRW_HORIZONTAL_STRIDE_1));
> -

Were 0,x,0 and 8,x,1 just a list of expected cases before or was it
toward some restriction? I'm not seeing anything in the documentation
that implies a restriction, so I'm guessing the former.