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

[Jason Ekstrand]

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.

Is Tue SIMD stuff unblocked now?
--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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150729/40f88be5/attachment.html>