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

[Iago Toral]

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? 

> 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