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

[Samuel Iglesias Gonsálvez]

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.

Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>

Sam

>        switch (reg->type) {
>        case BRW_REGISTER_TYPE_F:
>  	 brw_reg = brw_imm_f(reg->fixed_hw_reg.dw1.f);
>