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

[Iago Toral]

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

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 :)

>           }
>  
>           inst->remove(block);