[Mesa-dev] [PATCH 2/5] i965/fs: fix stride and type for hw_reg's in regs_read()

[Connor Abbott]

On Tue, Jul 14, 2015 at 6:02 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Connor Abbott <cwabbott0 at gmail.com> writes:
>
>> sources with file == HW_REG get all their information from the
>> fixed_hw_reg field, so we need to get the stride and type from there
>> when computing the size.
>>
>> Signed-off-by: Connor Abbott <connor.w.abbott at intel.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 24 ++++++++++++++++++------
>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 38b9095..64f093b 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -696,24 +696,36 @@ fs_inst::regs_read(int arg) const
>>        break;
>>     }
>>
>> +   unsigned stride;
>> +   enum brw_reg_type type;
>> +
>>     switch (src[arg].file) {
>>     case BAD_FILE:
>>     case UNIFORM:
>>     case IMM:
>>        return 1;
>> +
>>     case GRF:
>> +      stride = src[arg].stride;
>> +      type = src[arg].type;
>> +      break;
>> +
>>     case HW_REG:
>> -      if (src[arg].stride == 0) {
>> -         return 1;
>> -      } else {
>> -         int size = components * this->exec_size * type_sz(src[arg].type);
>> -         return DIV_ROUND_UP(size * src[arg].stride, 32);
>> -      }
>> +      stride = src[arg].fixed_hw_reg.hstride;
>> +      type = src[arg].fixed_hw_reg.type;
>> +      break;
>> +
>>     case MRF:
>>        unreachable("MRF registers are not allowed as sources");
>>     default:
>>        unreachable("Invalid register file");
>>     }
>> +
>> +   if (stride == 0)
>> +      return 1;
>> +
>> +   int size = components * this->exec_size * type_sz(type);
>> +   return DIV_ROUND_UP(size * stride, 32);
>
> I don't think this will work unfortunately, brw_reg::hstride is the log2
> of the actual stride, unlike fs_reg::stride.  Did I already mention I'm
> appalled by the fact that fs_reg has a number of fields with overlapping
> semantics but different representation, one or the other being
> applicable depending on the occasion.  I guess it would be more or less
> bearable if these data members were declared private and some reasonable
> abstraction was provided to access them.

I don't think anybody's happy with it, but refactoring that is it's
own can of worms.

>
> How do you like the attached patch?  It doesn't solve the fundamental
> problem but it seems to improve the situation slightly.

It seems fine to me... I was more paranoid about getting the type from
the fixed_hw_reg too, but brw_reg_from_fs_reg() in the generator we
have:

assert(reg->type == reg->fixed_hw_reg.type);

so it seems my paranoia wasn't justified. I'd like someone else who's
more experienced to take a look though. I suspect that others might
want to bikeshed about the name, but I don't have a better suggestion.

>
>>  }
>>
>>  bool
>> --
>> 2.4.3
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>