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

[Kenneth Graunke]

On Thursday, April 28, 2016 3:21:43 PM PDT Ian Romanick wrote:
> 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.
> 

I like Ian's suggestion.  It might also be reasonable to use REG_SIZE:

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

Either way,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> > +         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;
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160510/35da531a/attachment.sig>