[Mesa-dev] [PATCH 15/24] i965/fs: Set stride = 1 for vector immediate types.

[Matt Turner]

On Wed, Nov 11, 2015 at 1:02 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Monday, November 02, 2015 04:29:25 PM Matt Turner wrote:
>> The generator asserts that this is true (and presumably it's useful in
>> some optimization passes?) and the VF fs_reg constructors did this (by
>> virtue of the fact that it doesn't override what init() does).
>>
>> In the next commit, calling this constructor with brw_imm_* will
>> generate an IMM file register rather than a HW_REG, making this change
>> necessary to avoid breakage with existing uses of brw_imm_v().
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 91eaf61..92a9437 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -427,6 +427,12 @@ fs_reg::fs_reg(struct brw_reg reg) :
>>     this->subreg_offset = 0;
>>     this->reladdr = NULL;
>>     this->stride = 1;
>> +   if (this->file == IMM &&
>> +       (this->type != BRW_REGISTER_TYPE_V &&
>> +        this->type != BRW_REGISTER_TYPE_UV &&
>> +        this->type != BRW_REGISTER_TYPE_VF)) {
>> +      this->stride = 0;
>> +   }
>>  }
>>
>>  bool
>>
>
> It's confusing that your subject says "Set stride to 1 for vector
> immediate types" yet your code sets stride to 0.

Right, subject fail.

> I would suggest renaming the patch to
>
> "i965/fs: Set stride correctly for immediates in fs_reg(brw_reg)"
>
> and adding some text to the commit message like:
>
> "The fs_reg() constructors for immediates set stride to 0, except for
>  vector-immediates, which set stride to 1.  This patch makes the fs_reg
>  constructor that takes a brw_reg do likewise, so that stride is set
>  correctly for cases such as fs_reg(brw_imm_v(...))."

Sure, sounds good.

> Regardless,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>