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.
> Reviewed-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com>
>
Thanks!
> 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
