[Mesa-dev] [PATCH 16/57] i965/fs: Take into account trailing padding in regs_written() and regs_read().

[Iago Toral]

On Fri, 2016-09-09 at 13:03 -0700, Francisco Jerez wrote:
> Iago Toral <itoral at igalia.com> writes:
> 
> > 
> > On Wed, 2016-09-07 at 18:48 -0700, Francisco Jerez wrote:
> > > 
> > > This fixes regs_written() and regs_read() to return a more
> > > accurate
> > > value when the padding left between components due to a stride
> > > value
> > > greater than one causes the region bounds given by size_written
> > > or
> > > size_read to overflow into the next register.  This could become
> > > a
> > > problem in optimization passes that keep track of dataflow using
> > > fixed-size arrays with register granularity, because the overflow
> > > register (not actually accessed by the region) may not have been
> > > allocated at all which could lead to undefined memory access.
> > ah, nice catch! I am curious: did you find any cases where this was
> > creating a real problem or just something you noticed by
> > inspection? 
> > 
> Yeah, it definitely fixed real problems in a bunch of FP64 tests (I
> forgot which ones but I could dig it up if you're interested)

No, that's not necessary, I was simply curious because I had never seen
problems because of this issue,  but you already explain why below.

>  which
> would have regressed from the next commits that fix regs_written and
> regs_read to take into account misalignment -- The only reason this
> wasn't a problem before is that we weren't rounding up the register
> count correctly in most places...
> 
> > 
> > > 
> > > An alternative to this would be to subtract the trailing padding
> > > already during the calculation of fs_inst::size_read and
> > > ::size_written, but that would break code that currently assumes
> > > that
> > > ::size_read and _written are whole multiples of the component
> > > size,
> > > and would be hard to maintain looking forward because
> > > size_written is
> > > assigned from a bunch of different places.
> > Yeah, this would be a more problematic solution.
> > 
> > > 
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_ir_fs.h | 22 ++++++++++++++++++++-
> > > -
> > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > > b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > > index 4ac9baa..0be67b7 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > > @@ -192,6 +192,20 @@ reg_offset(const fs_reg &r)
> > >  }
> > >  
> > >  /**
> > > + * Return the amount of padding in bytes left unused between
> > > individual
> > > + * components of register \p r due to a (horizontal) stride
> > > value
> > > greater than
> > > + * one, or zero if components are tightly packed in the register
> > > file.
> > > + */
> > > +static inline unsigned
> > > +reg_padding(const fs_reg &r)
> > > +{
> > > +   const unsigned stride = ((r.file != ARF && r.file !=
> > > FIXED_GRF) ?
> > > r.stride :
> > > +                            r.hstride == 0 ? 0 :
> > > +                            1 << (r.hstride - 1));
> > > +   return (MAX2(1, stride) - 1) * type_sz(r.type);
> > > +}
> > > +
> > > +/**
> > >   * Return whether the register region starting at \p r and
> > > spanning
> > > \p dr
> > >   * bytes could potentially overlap the register region starting
> > > at
> > > \p s and
> > >   * spanning \p ds bytes.
> > > @@ -423,7 +437,9 @@ regs_written(const fs_inst *inst)
> > >  {
> > >     /* XXX - Take into account register-misaligned offsets
> > > correctly.
> > > */
> > >     assert(inst->dst.file != UNIFORM && inst->dst.file != IMM);
> > > -   return DIV_ROUND_UP(inst->size_written, REG_SIZE);
> > > +   return DIV_ROUND_UP(inst->size_written -
> > > +                       MIN2(inst->size_written,
> > > reg_padding(inst-
> > > > 
> > > > dst)),
> > > +                       REG_SIZE);
> > >  }
> > >  
> > >  /**
> > > @@ -438,7 +454,9 @@ regs_read(const fs_inst *inst, unsigned i)
> > >     /* XXX - Take into account register-misaligned offsets
> > > correctly.
> > > */
> > >     const unsigned reg_size =
> > >        inst->src[i].file == UNIFORM || inst->src[i].file == IMM ?
> > > 4 :
> > > REG_SIZE;
> > > -   return DIV_ROUND_UP(inst->size_read(i), reg_size);
> > > +   return DIV_ROUND_UP(inst->size_read(i) -
> > > +                       MIN2(inst->size_read(i),
> > > reg_padding(inst-
> > > > 
> > > > src[i])),
> > > +                       reg_size);
> > >  }
> > >  
> > >  #endif