[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 Jul 29, 2015 3:12 AM, "Francisco Jerez" <currojerez at riseup.net> wrote:
>>
>> 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).
>
> Then let's change the commit message to say that the execution controls
> don't matter.  With that, you can have my R-B on the last two.
>
Alright, I'll fix that.

> Is Tue SIMD stuff unblocked now?

Yeah, I already pushed most of it, thanks :)

> --Jason
>
>> >> ---
>> >>  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/d94bd0c7/attachment.sig>