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

[Francisco Jerez]

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

> On Fri, 2015-07-17 at 16:33 +0300, Francisco Jerez wrote:
>> 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.
>> 
>
> I was talking about adding extra parenthesis in the condition just for
> readability :-)
>
> assert(reg->stride == ((reg->type == BRW_REGISTER_TYPE_V ||
>                         reg->type == BRW_REGISTER_TYPE_UV ||
>                         reg->type == BRW_REGISTER_TYPE_VF) ? 1 : 0));
>
Hah, OK, I'll add the extra parentheses if you find it easier to read
that way.

Thanks.

> In any case, thanks for the explanation :-)
>
>> > Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> >
>> 
>> Thanks!
>> 
>
> You are welcome!
>
> Sam
>
>> > 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/a88270bb/attachment-0001.sig>