[Mesa-dev] [PATCH 1/4] i965: move subreg_offset to backend_reg

[Francisco Jerez]

Iago Toral Quiroga <itoral at igalia.com> writes:

> So we can access it in the vec4 backend to handle byte offsets into
> registers.

This change has deep implications in the meaning of the vec4 register
objects because the representation of register offsets is now split
between 'reg_offset' and 'subreg_offset', and there are a *lot* of
places that directly rely on the current representation of register
offsets, just grep the VEC4 back-end for 'reg_offset' -- Every
occurrence is now suspect of being inaccurate because the offset of a
register is no longer guaranteed to be 32B-aligned, so comparing
reg_offsets alone to find out whether two registers are equivalent or
whether they overlap is no longer sufficient.

Do you *really* need to make this representation change for FP64?  Or is
there some way around it?  ISTR that in the big VEC4 FP64 series you
ended up using backend_reg::subnr for this (which is just an undercover
version of subreg_offset so the same caveat applies :P) when you had to
address both halves of a single-precision register during SIMD lowering
(Can you remind me what the exact use-case was for that?).  To address
the ZW components of a double-precision vector this seems less of a
requirement because you can just use the logical (i.e. 64-bit-based)
swizzles at the IR level and only translate to physical
swizzles+offsets+strides during codegen time.

If the answer is that you definitely need this change, I think I'm going
to help you out with this because the change is going to be massive and
involve auditing the whole VEC4 back-end.  I think there are two lessons
to learn from the FS back-end:

 - Having the register offset split between two variables has been an
   endless source of bugs, because it's just too tempting to only take
   one of them into account and ignore the other.

 - It wouldn't be substantially more difficult to change reg_offset to
   be expressed in byte units instead, even if it involves going through
   every occurrence of reg_offset in the back-end, because adding a
   separate subreg_offset field invalidates every ocurrence of
   reg_offset in the back-end anyway.

> ---
>  src/mesa/drivers/dri/i965/brw_ir_fs.h  | 6 ------
>  src/mesa/drivers/dri/i965/brw_shader.h | 6 ++++++
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> index f214483..00fbace 100644
> --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> @@ -52,12 +52,6 @@ public:
>     /** Smear a channel of the reg to all channels. */
>     fs_reg &set_smear(unsigned subreg);
>  
> -   /**
> -    * Offset in bytes from the start of the register.  Values up to a
> -    * backend_reg::reg_offset unit are valid.
> -    */
> -   int subreg_offset;
> -
>     /** Register region horizontal stride */
>     uint8_t stride;
>  };
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
> index e61c080..ae23830 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.h
> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
> @@ -75,6 +75,12 @@ struct backend_reg : private brw_reg
>      */
>     uint16_t reg_offset;
>  
> +   /**
> +    * Offset in bytes from the start of the register.  Values up to a
> +    * backend_reg::reg_offset unit are valid.
> +    */
> +   uint16_t subreg_offset;
> +
>     using brw_reg::type;
>     using brw_reg::file;
>     using brw_reg::negate;
> -- 
> 2.7.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160823/88afc759/attachment.sig>