El 16/1/19 a las 0:55, Matt Turner escribió: > On Tue, Jan 15, 2019 at 8:58 AM Jason Ekstrand <ja...@jlekstrand.net> 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 <jmcasan...@igalia.com> >> 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 :(
This restriction only applies when we have a return register for the SEND so rlen is >= 1. If we had had an ">=" 127, we would be raising an exception for a legal destination register for a send in the cases like grf126 with rlen = 1. We could agree that another way of expressing it would be using dst_nr+rlen == 128. But in any case something over 128 would mean that something went wrong too. :) I agree that it also makes sense to include a general check for dst_nr + rlen <= 128 and the same for sources would make sense, although it isn't possible for our register allocator to assign that not existing combination it would be safer for changes of the register classes. > Not sure what I was thinking letting that patch through without a unit > test. I'll do that. I should have taken a look at test_eu_validate.cpp Regards, Chema _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev