Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes: > On Fri, 2015-07-17 at 16:33 +0300, Francisco Jerez wrote: >> Samuel Iglesias Gonsálvez <sigles...@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 <sigles...@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);
>> >>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
