[Mesa-dev] [PATCH 2/5] i965: Add support for swizzling arbitrary immediates to (brw_)swizzle().

[Iago Toral]

On Fri, 2016-02-26 at 22:02 -0800, Francisco Jerez wrote:
> Scalar immediates used to be handled correctly by swizzle() (as the
> identity) but since commit 58fa9d47b536403c4e3ca5d6a2495691338388fd it
> will corrupt the contents of the immediate.  Vector immediates were
> never handled correctly, but we had ad-hoc code to swizzle VF
> immediates in the vec4 copy propagation pass.  This takes care of
> swizzling V and UV in addition.
> ---
>  src/mesa/drivers/dri/i965/brw_eu.c      | 39 +++++++++++++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_ir_vec4.h |  6 ++++-
>  src/mesa/drivers/dri/i965/brw_reg.h     |  7 ++++--
>  3 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.c b/src/mesa/drivers/dri/i965/brw_eu.c
> index 40ec87d..36bdc7c 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu.c
> @@ -110,6 +110,45 @@ brw_swap_cmod(uint32_t cmod)
>     }
>  }
>  
> +/**
> + * Get the least significant bit offset of the i+1-th component of immediate
> + * type \p type.  For \p i equal to the two's complement of j, return the
> + * offset of the j-th component starting from the end of the vector.  For
> + * scalar register types return zero.
> + */
> +static unsigned
> +imm_shift(enum brw_reg_type type, unsigned i)
> +{
> +   if (type == BRW_REGISTER_TYPE_VF)
> +      return 8 * (i & 3);
> +   else if (type == BRW_REGISTER_TYPE_UV || type == BRW_REGISTER_TYPE_V)
> +      return 4 * (i & 7);
> +   else
> +      return 0;
> +}
> +
> +/**
> + * Swizzle an arbitrary immediate \p x of the given type according to the
> + * permutation specified as \p swz.
> + */
> +uint32_t
> +brw_swizzle_immediate(enum brw_reg_type type, uint32_t x, unsigned swz)
> +{
> +   if (imm_shift(type, 1)) {
> +      const unsigned n = 32 / imm_shift(type, 1);

You can assign imm_shift(type, 1) to a variable before the if and reuse
it here.

> +      uint32_t y = 0;
> +
> +      for (unsigned i = 0; i < n; i++)
> +         y |= x >> imm_shift(type, (i & ~3) + BRW_GET_SWZ(swz, i & 3))
> +                << imm_shift(type, ~0u)
> +                >> imm_shift(type, ~0u - i);
> +

Ugh, took me a while to understand what this was doing even with the
comment in imm_shift(). Another comment here explaining what this series
of operations are doing might save future readers some time... ;)

The implementation looks correct to me:
Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

> +      return y;
> +   } else {
> +      return x;
> +   }
> +}
> +
>  void
>  brw_set_default_exec_size(struct brw_codegen *p, unsigned value)
>  {
> diff --git a/src/mesa/drivers/dri/i965/brw_ir_vec4.h b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> index 660beca..2b6872e 100644
> --- a/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> @@ -76,7 +76,11 @@ offset(src_reg reg, unsigned delta)
>  static inline src_reg
>  swizzle(src_reg reg, unsigned swizzle)
>  {
> -   reg.swizzle = brw_compose_swizzle(swizzle, reg.swizzle);
> +   if (reg.file == IMM)
> +      reg.ud = brw_swizzle_immediate(reg.type, reg.ud, swizzle);
> +   else
> +      reg.swizzle = brw_compose_swizzle(swizzle, reg.swizzle);
> +
>     return reg;
>  }
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
> index a4bcfca..74ff67f 100644
> --- a/src/mesa/drivers/dri/i965/brw_reg.h
> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
> @@ -223,6 +223,7 @@ enum PACKED brw_reg_type {
>  unsigned brw_reg_type_to_hw_type(const struct brw_device_info *devinfo,
>                                   enum brw_reg_type type, enum brw_reg_file file);
>  const char *brw_reg_type_letters(unsigned brw_reg_type);
> +uint32_t brw_swizzle_immediate(enum brw_reg_type type, uint32_t x, unsigned swz);
>  
>  #define REG_SIZE (8*4)
>  
> @@ -876,9 +877,11 @@ get_element_d(struct brw_reg reg, unsigned elt)
>  static inline struct brw_reg
>  brw_swizzle(struct brw_reg reg, unsigned swz)
>  {
> -   assert(reg.file != BRW_IMMEDIATE_VALUE);
> +   if (reg.file == BRW_IMMEDIATE_VALUE)
> +      reg.ud = brw_swizzle_immediate(reg.type, reg.ud, swz);
> +   else
> +      reg.swizzle = brw_compose_swizzle(swz, reg.swizzle);
>  
> -   reg.swizzle = brw_compose_swizzle(swz, reg.swizzle);
>     return reg;
>  }
>