On Tue, Jan 15, 2019 at 8:58 AM Jason Ekstrand <[email protected]> wrote: > > Previously, we only applied the fix to shaders with a dispatch mode of > SIMD8 but the code it relies on for SIMD16 mode only applies to SIMD16 > instructions. If you have a SIMD8 instruction in a SIMD16 shader, > neither would trigger and the restriction could still be hit. > > Cc: Jose Maria Casanova Crespo <[email protected]> > Fixes: 232ed8980217dd "i965/fs: Register allocator shoudn't use grf127..." > --- > src/intel/compiler/brw_fs_reg_allocate.cpp | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp > b/src/intel/compiler/brw_fs_reg_allocate.cpp > index 5db5242452e..ec743f9b5bf 100644 > --- a/src/intel/compiler/brw_fs_reg_allocate.cpp. > +++ b/src/intel/compiler/brw_fs_reg_allocate.cpp > @@ -667,15 +667,14 @@ fs_visitor::assign_regs(bool allow_spilling, bool > spill_all) > * 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. > + * We don't apply it to SIMD16 instructions because previous code > avoids > + * any register overlap between sources and destination. > */ > ra_set_node_reg(g, grf127_send_hack_node, 127); > - if (dispatch_width == 8) { > - 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); > - } > + foreach_block_and_inst(block, fs_inst, inst, cfg) { > + if (inst->exec_size < 16 && inst->is_send_from_grf() && > + inst->dst.file == VGRF) > + ra_add_node_interference(g, inst->dst.nr, grf127_send_hack_node); > } >
Did the code in brw_eu_validate.c catch the case you found? In fact, that code looks wrong: | (brw_inst_dst_da_reg_nr(devinfo, inst) + | brw_inst_rlen(devinfo, inst) > 127) && I think > should be >=. And maybe we should have a separate case earlier that checks that dst_nr+rlen actually fits in registers, and then change > to just ==. FFS :( Not sure what I was thinking letting that patch through without a unit test. I'll do that. _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
