[Mesa-dev] [PATCH 08/14] i965/fs: Initialize a builder explicitly in the gen4 send dependency work-arounds.

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Instead of relying on the default one.  This shouldn't lead to any
>> functional changes because DEP_RESOLVE_MOV overrides the execution
>> controls of the instruction anyway.
>
> Actually, DEP_RESOLVE_MOV calls half() on the builder which doesn't
> make any sense.  I think the pre-builder intention, based on the
> comment, was force_uncompressed and to use a SIMD8 instruction so that
> it adds as few deps as possible.  I'm not sure what it's actually
> doing now.  In any case, saying that it overrides everything isn't
> right.
>
Pre-builder it was doing the same it's doing now -- Emit an 8-wide
instruction regardless of the dispatch width of the shader.
fs_builder::half(0) is an alias for ::group(8, 0) which simply selects
the first 8-channel group of the channel enables and will cause the
instruction to be uncompressed during code generation.

You're right that it doesn't override execution controls of the
instruction other than exec_size, but that's because it doesn't need to.
force_writemask_all and force_sechalf are fully irrelevant for what the
emitted MOV is intended to do: stall the pipeline until the instruction
that was writing the GRF provided as argument retires, which is going to
happen regardless of the EMask of the MOV instruction (even if it's zero
it should cause a stall because pre-IVB hardware didn't implement
shoot-down of instructions with zero EMask).

>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 71d372c..8bc9372 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -2827,7 +2827,8 @@ fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
>>        if (block->start() == scan_inst) {
>>           for (int i = 0; i < write_len; i++) {
>>              if (needs_dep[i])
>> -               DEP_RESOLVE_MOV(bld.at(block, inst), first_write_grf + i);
>> +               DEP_RESOLVE_MOV(fs_builder(this, block, inst),
>> +                               first_write_grf + i);
>>           }
>>           return;
>>        }
>> @@ -2843,7 +2844,7 @@ fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
>>              if (reg >= first_write_grf &&
>>                  reg < first_write_grf + write_len &&
>>                  needs_dep[reg - first_write_grf]) {
>> -               DEP_RESOLVE_MOV(bld.at(block, inst), reg);
>> +               DEP_RESOLVE_MOV(fs_builder(this, block, inst), reg);
>>                 needs_dep[reg - first_write_grf] = false;
>>                 if (scan_inst->exec_size == 16)
>>                    needs_dep[reg - first_write_grf + 1] = false;
>> @@ -2890,7 +2891,8 @@ fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block, fs_ins
>>        if (block->end() == scan_inst) {
>>           for (int i = 0; i < write_len; i++) {
>>              if (needs_dep[i])
>> -               DEP_RESOLVE_MOV(bld.at(block, scan_inst), first_write_grf + i);
>> +               DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
>> +                               first_write_grf + i);
>>           }
>>           return;
>>        }
>> @@ -2905,7 +2907,8 @@ fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block, fs_ins
>>            scan_inst->dst.reg >= first_write_grf &&
>>            scan_inst->dst.reg < first_write_grf + write_len &&
>>            needs_dep[scan_inst->dst.reg - first_write_grf]) {
>> -         DEP_RESOLVE_MOV(bld.at(block, scan_inst), scan_inst->dst.reg);
>> +         DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
>> +                         scan_inst->dst.reg);
>>           needs_dep[scan_inst->dst.reg - first_write_grf] = false;
>>        }
>>
>> --
>> 2.4.6
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150729/23e016bf/attachment.sig>