[Mesa-dev] [PATCH] intel/fs: Do the grf127 hack on SIMD8 instructions in SIMD16 mode

[Chema Casanova]

El 16/1/19 a las 0:55, Matt Turner escribió:
> On Tue, Jan 15, 2019 at 8:58 AM Jason Ekstrand <jason at 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 <jmcasanova at 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