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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev