[Mesa-dev] [PATCH 04/15] i965/fs: half exec_size when dealing with 64 bits attributes

[Ian Romanick]

On 04/28/2016 01:40 PM, Antia Puentes wrote:
> From: Alejandro Piñeiro <apinheiro at igalia.com>
> 
> The HW has a restriction that only vertical stride may cross register
> boundaries. Until now this was only handled on VGRFs at
> rw_reg_from_fs_reg, but it is also needed for attributes.
> 
> This can be seen as the equivalent of commit 552cfa9 but for
> attributes. Take a look to the git message on that commit for further
> info.
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index ce84d0a..98cbf9f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -1844,11 +1844,30 @@ fs_visitor::convert_attr_sources_to_hw_regs(fs_inst *inst)
>                     inst->src[i].nr +
>                     inst->src[i].reg_offset;
>  
> -         unsigned width = inst->src[i].stride == 0 ? 1 : inst->exec_size;
> +         unsigned exec_size;

Blank line here.

> +         /* As explained at brw_reg_from_fs_reg, From the Haswell PRM:
> +          *
> +          * VertStride must be used to cross GRF register boundaries. This
> +          * rule implies that elements within a 'Width' cannot cross GRF
> +          * boundaries.
> +          *
> +          * So, for registers that are large enough, we have to split the exec
> +          * size in two and trust the compression state to sort it out.
> +          */
> +         unsigned total_size = inst->exec_size *
> +                               inst->src[i].stride *
> +                               type_sz(inst->src[i].type);

          const unsigned total_size = ... ;

I think the code below is better as

          assert(total_size <= 64);

          const unsigned exec_size = (total_size <= 32) ? inst->exec_size : inst->exec_size / 2;

I'll defer to one of the people who work more in the back end.

> +         if (total_size <= 32) {
> +            exec_size = inst->exec_size;
> +         } else {
> +            assert(total_size / 2 <= 32);
> +            exec_size = inst->exec_size / 2;
> +         }
> +         unsigned width = inst->src[i].stride == 0 ? 1 : exec_size;
>           struct brw_reg reg =
>              stride(byte_offset(retype(brw_vec8_grf(grf, 0), inst->src[i].type),
>                                 inst->src[i].subreg_offset),
> -                   inst->exec_size * inst->src[i].stride,
> +                   exec_size * inst->src[i].stride,
>                     width, inst->src[i].stride);
>           reg.abs = inst->src[i].abs;
>           reg.negate = inst->src[i].negate;
>