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

[Jason Ekstrand]

On January 15, 2019 17:55:31 Matt Turner <mattst88 at gmail.com> wrote:

> 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?

Yes, it did. It was a fairly simple case; it just occurred as a SIMD8 
instruction in a SIMD16 program.

> 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.