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

[Jason Ekstrand]

On Thu, Sep 17, 2015 at 1:09 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> 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:

Sorry about that, I failed at rebasing...

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

Thanks for all your reviewing!
--Jason

>> 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.

Sure.

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