[Mesa-dev] [PATCH] i965/vec4: select predicate based on writemask for sel emissions

[Matt Turner]

On Fri, Oct 23, 2015 at 7:17 AM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
> Equivalent to commit 4eebeb but with sel operations. In this case

That commit sha doesn't exist. I suspect you meant commit 8ac3b525c.

> we select the PredCtrl based on the writemask.
>
> This change allows cmod propagation to optimize out several
> instructions.

Okay, so this patch helps a case like:

 cmp.g.f0.0 vgrf32.0.x:F, vgrf31.xxxx:F, 0.000000F
 cmp.nz.f0.0 null.x:D, vgrf32.xxxx:D, 0D
 (+f0.0) sel vgrf2.0.x:F, |vgrf27.xxxx|:F, 0.000000F

where our code thinks the SEL reads all four channels of the flag
register when it actually only reads .x because of the writemask.

Adding something like this to the commit message would be good.

> Shader-db numbers:
> total instructions in shared programs: 6235835 -> 6228008 (-0.13%)
> instructions in affected programs:     219850 -> 212023 (-3.56%)
> total loops in shared programs:        1979 -> 1979 (0.00%)
> helped:                                1192
> HURT:                                  0
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> index 0f04f65..bc86be6 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> @@ -1437,8 +1437,24 @@ vec4_visitor::nir_emit_alu(nir_alu_instr *instr)
>     case nir_op_bcsel:
>        emit(CMP(dst_null_d(), op[0], src_reg(0), BRW_CONDITIONAL_NZ));
>        inst = emit(BRW_OPCODE_SEL, dst, op[1], op[2]);
> -      inst->predicate = BRW_PREDICATE_NORMAL;
> +      switch (dst.writemask) {
> +      case WRITEMASK_X:
> +         inst->predicate = BRW_PREDICATE_ALIGN16_REPLICATE_X;
> +         break;
> +      case WRITEMASK_Y:
> +         inst->predicate = BRW_PREDICATE_ALIGN16_REPLICATE_Y;
> +         break;
> +      case WRITEMASK_Z:
> +         inst->predicate = BRW_PREDICATE_ALIGN16_REPLICATE_Z;
> +         break;
> +      case WRITEMASK_W:
> +         inst->predicate = BRW_PREDICATE_ALIGN16_REPLICATE_W;
> +         break;
> +      default:
> +         inst->predicate = BRW_PREDICATE_NORMAL;
>        break;

This break, and the one two lines below it are indented incorrectly.

> +      }
> +   break;
>
>     case nir_op_fdot_replicated2:
>        inst = emit(BRW_OPCODE_DP2, dst, op[0], op[1]);
> --
> 2.1.4
>

With the commit message fixed/updated and the breaks properly indented,

Reviewed-by: Matt Turner <mattst88 at gmail.com>

Thanks!