[Mesa-dev] [PATCH 1/3] i965/fs: Fix stride for immediate registers.

[Francisco Jerez]

Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:

> On 16/07/15 17:33, Francisco Jerez wrote:
>> When the width field was removed from fs_reg the BROADCAST handling
>> code in opt_algebraic() started to miss a number of trivial
>> optimization cases resulting in the ugly indirect-addressing sequence
>> to be emitted unnecessarily for some variable-indexed texturing and
>> UBO loads regardless of one of the sources of BROADCAST being
>> immediate.  Apparently the reason was that we were setting the stride
>> field to one for immediates even though they are typically uniform.
>> Width used to be set to one too which is why this optimization used to
>> work previously until the "reg.width == 1" check was removed.
>> 
>> The stride field of vector immediates is intentionally left equal to
>> one, because they are strictly speaking not uniform.  The assertion in
>> fs_generator makes sure that immediates have the expected stride as
>> consistency check.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp           | 3 +++
>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 4 ++++
>>  2 files changed, 7 insertions(+)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index ff0675d..537ccbe 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -362,6 +362,7 @@ fs_reg::fs_reg(float f)
>>     init();
>>     this->file = IMM;
>>     this->type = BRW_REGISTER_TYPE_F;
>> +   this->stride = 0;
>>     this->fixed_hw_reg.dw1.f = f;
>>  }
>>  
>> @@ -371,6 +372,7 @@ fs_reg::fs_reg(int32_t i)
>>     init();
>>     this->file = IMM;
>>     this->type = BRW_REGISTER_TYPE_D;
>> +   this->stride = 0;
>>     this->fixed_hw_reg.dw1.d = i;
>>  }
>>  
>> @@ -380,6 +382,7 @@ fs_reg::fs_reg(uint32_t u)
>>     init();
>>     this->file = IMM;
>>     this->type = BRW_REGISTER_TYPE_UD;
>> +   this->stride = 0;
>>     this->fixed_hw_reg.dw1.ud = u;
>>  }
>>  
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> index bae7216..8a3af47 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> @@ -79,6 +79,10 @@ brw_reg_from_fs_reg(fs_inst *inst, fs_reg *reg)
>>        brw_reg = byte_offset(brw_reg, reg->subreg_offset);
>>        break;
>>     case IMM:
>> +      assert(reg->stride == (reg->type == BRW_REGISTER_TYPE_V ||
>> +                             reg->type == BRW_REGISTER_TYPE_UV ||
>> +                             reg->type == BRW_REGISTER_TYPE_VF ? 1 : 0));
>> +
>
> Just nitpicking: I would put extra parenthesis to enclose the whole
> condition.
>
Hah, in fact there is a good reason to put the whole ?: expression
around parenthesis rather than the condition only: The ternary operator
has one of the lowest precedences (one could argue it's dangerously low
-- it has the same precedence as the assignment operator!), which means
it's far more likely to fall into the opposite mistake of letting the
ternary operator bind too loosely as would have happened in this patch
if I had put the parenthesis around the condition instead:

|      assert(reg->stride == (reg->type == BRW_REGISTER_TYPE_V ||
|                             reg->type == BRW_REGISTER_TYPE_UV ||
|                             reg->type == BRW_REGISTER_TYPE_VF) ? 1 : 0);

would be parsed as:

|      assert((reg->stride == (reg->type == BRW_REGISTER_TYPE_V ||
|                              reg->type == BRW_REGISTER_TYPE_UV ||
|                              reg->type == BRW_REGISTER_TYPE_VF)) ? 1 : 0);

which isn't what I intended but *happens* to be equivalent by pure luck
because "boolean-expression ? 1 : 0" is in fact a no-op -- In general
it isn't. :)

The opposite case is almost impossible: Without parenthesis around the
condition it will only ever bind more tightly than intended if you want
to do an assignment or use the comma operator in the condition, which is
hardly ever the case so the parenthesis around the condition are almost
always redundant.

> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>

Thanks!

> Sam
>
>>        switch (reg->type) {
>>        case BRW_REGISTER_TYPE_F:
>>  	 brw_reg = brw_imm_f(reg->fixed_hw_reg.dw1.f);
>> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150717/45351c8a/attachment.sig>