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

[Francisco Jerez]

Connor Abbott <cwabbott0 at gmail.com> writes:

> 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.
>

Sure, it would be a pile of work, but I think it should be quite
straightforward in principle.  We could just punt fixed_hw_reg and
replace it with an ARF file and a fixed-GRF file using the same fields
normal regististers use.  For immediates we'd have to add to add a union
with float/unsigned/int fields similar to brw_reg::dw1.

>>
>> 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.
>

Yeah, your paranoia was definitely justified, it's essentially the same
problem.  It actually led to actual bugs in the past which is why I
added that assertion and changed retype() to keep the type of the
fixed_hw_reg in sync with the normal type...

>>
>>>  }
>>>
>>>  bool
>>> --
>>> 2.4.3
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150715/bf70e64e/attachment.sig>