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

[Samuel Iglesias Gonsálvez]

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));

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: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150717/677c590d/attachment.sig>