[Mesa-stable] [Mesa-dev] [PATCH] i965/fs: unspills shoudn't use grf127 as dest since Gen8+

[Chema Casanova]

El 12/07/18 a las 03:23, Caio Marcelo de Oliveira Filho escribió:
> On Wed, Jul 11, 2018 at 06:03:05PM +0200, Jose Maria Casanova Crespo wrote:
>> At 232ed8980217dd65ab0925df28156f565b94b2e5 "i965/fs: Register allocator
>> shoudn't use grf127 for sends dest" we didn't take into account the case
>> of SEND instructions that are not send_from_grf. But since Gen7+ although
>> the backend still uses MRFs internally for sends they are finally asigned
>> to a GRFs.
> 
> Typo "assigned".

Fixed.

>> In the case of unspills the backend assigns directly as source its
>> destination because it is suppose to be available. So we always have a
>> source-destination overlap. If the reg_allocator asigns registers that
> 
> Typo "assigns".

Fixed.

>> include de grf127 we fail the validation rule that affects Gen8+
> 
> Typo "the".

Fixed.

>> "r127 must not be used for return address when there is a src and dest
>> overlap in send instruction."
>>
>> So this patch activates the grf127_send_hack_node for Gen8+ and if we have
>> any register spilled we add interferences to the destination of the unspill
>> operations.
> 
> I've spent some time testing why this patch was still not covering all
> the cases yet. The opt_bank_conflicts() optimization, that runs after
> the register allocation, was moving things around, causing the r127 to
> be used in the condition we were avoiding it.
> 
> The code there already has the idea of not touching certain registers,
> so we should add something like
> 
>       /* At Intel Broadwell PRM, vol 07, section "Instruction Set Reference",
>        * subsection "EUISA Instructions", Send Message (page 990):
>        *
>        * "r127 must not be used for return address when there is a src and
>        * dest overlap in send instruction."
>        *
>        * Register allocation ensures that, so don't move 127 around to avoid
>        * breaking that property.
>        */ 
>       if (v->devinfo->gen >= 8)
>          constrained[p.atom_of_reg(127)] = true;
> 
> to function shader_reg_constraints() in
> brw_fs_bank_conflicts.cpp. This fixes the crashes I was seeing in
> shader-db.
> 
> With the change to bank conflicts and the typos/style fixed, this
> patch is


Good finding. I like the clean and simple solution. At that point of
optimizing back conflicts I don't find a better way to don't mess with
grf127, although we are forbidding legal permutations when not SEND
instructions are in place. I've just putting the your code after the
constrains for reg0 and reg1.

I've also confirmed that that that I run a full shader-db without
crashes caused by this validation rule and the performance impact of the
patch doesn't seem to be too much taking into account that we are
avoiding generating instructions with undefined return values.

total instructions in shared programs: 14867211 -> 14867218 (<.01%)
instructions in affected programs: 5314 -> 5321 (0.13%)
helped: 1
HURT: 1

total cycles in shared programs: 537925161 -> 537923248 (<.01%)
cycles in affected programs: 44939136 -> 44937223 (<.01%)
helped: 10
HURT: 23

total spills in shared programs: 7789 -> 7790 (0.01%)
spills in affected programs: 107 -> 108 (0.93%)
helped: 0
HURT: 1

total fills in shared programs: 10555 -> 10557 (0.02%)
fills in affected programs: 155 -> 157 (1.29%)
helped: 0
HURT: 1

> Reviewed-by: Caio Marcelo de Oliveira Filho <caio.oliveira at intel.com>

Thanks for the review.

Chema

> 
> Reviewed-by: Caio Marcelo de Oliveira Filho <caio.oliveira at intel.com>
> 
> 
>> +      if (spilled_any_registers) {
>> +         foreach_block_and_inst(block, fs_inst, inst, cfg) {
>> +            if ((inst->opcode == SHADER_OPCODE_GEN7_SCRATCH_READ ||
>> +                inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_READ) &&
>> +                inst->dst.file ==VGRF) {
> 
> Missing space after the "==".
> 
>> +               ra_add_node_interference(g, inst->dst.nr, grf127_send_hack_node);
>> +            }
>>           }
>>        }
>>     }
>>  
>> +
> 
> Extra newline?
> 
> 
> Thanks,
> Caio
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>