[Mesa-dev] [PATCH v2 4/4] i965/vec4: Use nir_move_vec_src_uses_to_dest

[Eduardo Lima Mitev]

On 09/15/2015 10:44 PM, Jason Ekstrand wrote:
> The idea here is not that it gives register coalescing a little bit of a
> helping hand.  It doesn't actually fix the coalescing problems, but it
> seems to help a good bit.
> 
> Shader-db results for vec4 programs on Haswell:
> 
>    total instructions in shared programs: 1746280 -> 1683959 (-3.57%)
>    instructions in affected programs:     1259166 -> 1196845 (-4.95%)
>    helped:                                11363
>    HURT:                                  148
> 

I get exactly the same numbers on my Haswell too. Nice!

Some comments below.

> v2 (Jason Ekstrand):
>  - Run nir_move_vec_src_uses_to_dest after going out of SSA
>  - New shader-db numbers
> ---
>  src/glsl/nir/nir_move_vec_src_uses_to_dest.c | 16 ++++++++++++----
>  src/mesa/drivers/dri/i965/brw_nir.c          |  2 ++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/src/glsl/nir/nir_move_vec_src_uses_to_dest.c b/src/glsl/nir/nir_move_vec_src_uses_to_dest.c
> index 6e248a2..4c9032d 100644
> --- a/src/glsl/nir/nir_move_vec_src_uses_to_dest.c
> +++ b/src/glsl/nir/nir_move_vec_src_uses_to_dest.c
> @@ -79,17 +79,23 @@ move_vec_src_uses_to_dest_block(nir_block *block, void *shader)
>           continue; /* The loop */
>        }
>  
> +      /* Can't handle non-SSA vec operations */
> +      if (!vec->dest.dest.is_ssa)
> +         continue;
> +
>        /* Can't handle saturation */
>        if (vec->dest.saturate)
>           continue;
>  
> -      assert(vec->dest.dest.is_ssa);
> -
>        /* First, mark all of the sources we are going to consider for rewriting
>         * to the destination
>         */
>        int srcs_remaining = 0;
>        for (unsigned i = 0; i < nir_op_infos[vec->op].num_inputs; i++) {
> +         /* We can't rewrite a source if it's not in SSA form */
> +         if (!vec->src[i].src.is_ssa)
> +            continue;
> +
>           /* We can't rewrite a source if it has modifiers */
>           if (vec->src[i].abs || vec->src[i].negate)
>              continue;
> @@ -97,9 +103,11 @@ move_vec_src_uses_to_dest_block(nir_block *block, void *shader)
>           srcs_remaining |= 1 << i;
>        }
>  
> -      for (unsigned i; i = ffs(srcs_remaining) - 1, srcs_remaining;) {
> -         assert(vec->src[i].src.is_ssa);
> +      /* We can't actually do anything with this instruction */
> +      if (srcs_remaining == 0)
> +         continue;
>  
> +      for (unsigned i; i = ffs(srcs_remaining) - 1, srcs_remaining;) {
>           int8_t swizzle[4] = { -1, -1, -1, -1 };
>  
>           for (unsigned j = i; j < nir_op_infos[vec->op].num_inputs; j++) {
>

I think all hunks above should be squashed into previous patch (3/4).
With that, both patches 3/4 and this one 4/4 are:

Reviewed-by: Eduardo Lima Mitev <elima at igalia.com>

> diff --git a/src/mesa/drivers/dri/i965/brw_nir.c b/src/mesa/drivers/dri/i965/brw_nir.c
> index f326b23..8568988 100644
> --- a/src/mesa/drivers/dri/i965/brw_nir.c
> +++ b/src/mesa/drivers/dri/i965/brw_nir.c
> @@ -187,6 +187,8 @@ brw_create_nir(struct brw_context *brw,
>     nir_validate_shader(nir);
>  
>     if (!is_scalar) {
> +      nir_move_vec_src_uses_to_dest(nir);
> +      nir_validate_shader(nir);

A blank line here maybe? Not that I care much.

>        nir_lower_vec_to_movs(nir);
>        nir_validate_shader(nir);
>     }
> 

Eduardo