[Mesa-dev] [PATCH v3 02/48] intel/fs: Be more explicit about our placement of [un]zip

[Jason Ekstrand]

On Thu, Oct 26, 2017 at 1:39 AM, Iago Toral <itoral at igalia.com> wrote:

> On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> > Before, we were careful to place the zip after the last of the split
> > instructions but did unzip on-demand.  This changes things so that
> > the
> > unzips go before all of the split instructions and the unzip comes
> > explicitly after all the split instructions.  As a side-effect of
> > this
> > change, we now emit the split instruction from highest SIMD group to
> > lowest instead of low to high.  We could have kept the old behavior,
> > but
> > it shouldn't matter and this made the code easier.
>
> Took me a while to review that all this checked out, but it seems to do
> what you explain here. I have a couple of minor comments below.
>
>
> > Cc: mesa-stable at lists.freedesktop.org
> > ---
> >  src/intel/compiler/brw_fs.cpp | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_fs.cpp
> > b/src/intel/compiler/brw_fs.cpp
> > index 49ca58d..ef36af9 100644
> > --- a/src/intel/compiler/brw_fs.cpp
> > +++ b/src/intel/compiler/brw_fs.cpp
> > @@ -5186,6 +5186,7 @@ fs_visitor::lower_simd_width()
> >
> >           assert(!inst->writes_accumulator && !inst->mlen);
> >
> > +         exec_node * const after_inst = inst->next;
>
> I think code style should be:
>
> exec_node *const after_inst = inst->next
>

Sure.


> not a big deal anyway...
>
> >           for (unsigned i = 0; i < n; i++) {
> >              /* Emit a copy of the original instruction with the
> > lowered width.
> >               * If the EOT flag was set throw it away except for the
> > last
> > @@ -5193,7 +5194,7 @@ fs_visitor::lower_simd_width()
> >               */
> >              fs_inst split_inst = *inst;
> >              split_inst.exec_size = lower_width;
> > -            split_inst.eot = inst->eot && i == n - 1;
> > +            split_inst.eot = inst->eot && i == 0;
> >
> >              /* Select the correct channel enables for the i-th
> > group, then
> >               * transform the sources and destination and emit the
> > lowered
> > @@ -5205,11 +5206,11 @@ fs_visitor::lower_simd_width()
> >                 split_inst.src[j] = emit_unzip(lbld.at(block, inst),
> > inst, j);
> >
> >              split_inst.dst = emit_zip(lbld.at(block, inst),
> > -                                      lbld.at(block, inst->next),
> > inst);
> > +                                      lbld.at(block, after_inst),
> > inst);
> >              split_inst.size_written =
> >                 split_inst.dst.component_size(lower_width) *
> > dst_size;
> >
> > -            lbld.emit(split_inst);
> > +            lbld.at(block, inst->next).emit(split_inst);
>
> It was not immediately obvious to me that inst->next here is not the
> same as 'after_inst' due to the emit_zip() above, I am not sure if that
> deserves a comment though, I'll let you decide :)
>

I've inserted a ~10 line comment. :)


> >           }
> >
> >           inst->remove(block);
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171026/0a1d6df6/attachment.html>