[Mesa-dev] [RFC] i965/fs: Generalize grf127 hack to dispatch_width > 8

[Chema Casanova]

El 11/07/18 a las 03:50, Caio Marcelo de Oliveira Filho escribió:
> Change the hack to always apply, adjusting the register number
> according to the dispatch_width.
> 
> The original change assumed that given for dispatch_width > 8 we
> already prevent the overlap of source and destination for send, it
> would not be necessary to explicitly add an interference with a
> register that covers r127.
> 
> The problem is that the code for spilling registers ends up generating
> scratch reads, that in Gen7+ will reuse the destination register,
> causing a send with both source and destination overlaping. So prevent
> r127 (or the overlapping wider register) to be used as destination for
> sends.
> 
> This patch fixes piglit test
> tests/spec/arb_compute_shader/linker/bug-93840.shader_test.
> 
> Fixes: 232ed898021 "i965/fs: Register allocator shoudn't use grf127 for sends dest"
> ---
> 
> After more digging on the piglit failure, I came up with this
> patch. I'm still seeing crashes with for some shader-db executions
> (master have them too), but didn't have time today to drill into them
> 
>  src/intel/compiler/brw_fs_reg_allocate.cpp | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp b/src/intel/compiler/brw_fs_reg_allocate.cpp
> index 59e047483c0..417ddeba09c 100644
> --- a/src/intel/compiler/brw_fs_reg_allocate.cpp
> +++ b/src/intel/compiler/brw_fs_reg_allocate.cpp
> @@ -549,7 +549,7 @@ fs_visitor::assign_regs(bool allow_spilling, bool spill_all)
>     if (devinfo->gen >= 7)
>        node_count += BRW_MAX_GRF - GEN7_MRF_HACK_START;
>     int grf127_send_hack_node = node_count;
> -   if (devinfo->gen >= 8 && dispatch_width == 8)
> +   if (devinfo->gen >= 8 && dispatch_width >= 8)
>        node_count ++;
>     struct ra_graph *g =
>        ra_alloc_interference_graph(compiler->fs_reg_sets[rsi].regs, node_count);
> @@ -656,7 +656,7 @@ fs_visitor::assign_regs(bool allow_spilling, bool spill_all)
>        }
>     }
>  
> -   if (devinfo->gen >= 8 && dispatch_width == 8) {
> +   if (devinfo->gen >= 8 && dispatch_width >= 8) {
>        /* At Intel Broadwell PRM, vol 07, section "Instruction Set Reference",
>         * subsection "EUISA Instructions", Send Message (page 990):
>         *
> @@ -665,12 +665,9 @@ fs_visitor::assign_regs(bool allow_spilling, bool spill_all)
>         *
>         * We are avoiding using grf127 as part of the destination of send
>         * messages adding a node interference to the grf127_send_hack_node.
> -       * This node has a fixed asignment to grf127.
> -       *
> -       * We don't apply it to SIMD16 because previous code avoids any register
> -       * overlap between sources and destination.
> +       * This node has a fixed assignment that overlaps with grf127.
>         */
> -      ra_set_node_reg(g, grf127_send_hack_node, 127);
> +      ra_set_node_reg(g, grf127_send_hack_node, 128 - reg_width);

This configuration is more restrictive than needed. The original code
just avoids any register with any length that uses the physical register
grf127. Your code works for SIMD16, but as you are setting conflicts
with grf126 in SIMD16, you are forbidding the use of grf125 using with
regsize=2, and the same with grf123 with size 4, when this options never
use grf127. You don't need to take care of the reg_width here, just
about which physical register you can not use.

At brw_alloc_reg_set() you can check how the different registers are
defined using classes are used for different sizes. It also configures
the conflicts among the registers with different sizes and the physical
register.

So if at this point you create a node assigned to a physical register
you have conflicts with all the logical registers with any size that
overlap with it.

>        foreach_block_and_inst(block, fs_inst, inst, cfg) {
>           if (inst->is_send_from_grf() && inst->dst.file == VGRF) {
>              ra_add_node_interference(g, inst->dst.nr, grf127_send_hack_node);
> 

The issue here is that the unspill instructions aren't in the list of
the is_send_from_grf. I thought we could update is_send_from_grf to
include the read/write scratch operations but finally I think that it
didn't have sense because  the source at this point is an MRF that will
be finally assigned to a GRF on Gen7+.

I've sent a patch with my solution that I think solves the case of
unspill that is creating this problem, but maybe we need to think if
there are more SEND instructions that could have this problem because of
using the MRF as source.

Chema