On Thu, Oct 26, 2017 at 1:39 AM, Iago Toral <ito...@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-sta...@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);
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
