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

[Francisco Jerez]

Iago Toral <itoral at igalia.com> writes:

> 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.
>

Heh, I did that mainly out of laziness because it was less noise than
declaring a new variable.  It shouldn't make any practical difference
-- Or was your suggestion on purely stylistic grounds?

>> +      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... ;)
>
Sure, I'll throw in some more comments.

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

>> +      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;
>>  }
>>  
-------------- 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/20160229/41693e0d/attachment.sig>