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

[Caio Marcelo de Oliveira Filho]

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


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


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

Typo "the".


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

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