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));
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: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
